Re: [PATCH] Add regression tests of ecpg command notice (error / warning)
On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi wrote: > > Hi hackers, > > When I wrote patch of ecpg command notice bug, I recognized needs of > regression tests for ecpg command notices and I say that I write the > tests. > > https://commitfest.postgresql.org/52/5497/ > https://www.postgresql.org/message-id/0efab1f6-5d8d-451f-a7dc-ef9c73ba9e02%40oss.nttdata.com > > This mail is about patch of the tests. > > Patch is attached to this mail : ecpg_cmd_notice_regress_test.patch > This patch passed CI. > https://cirrus-ci.com/build/4861827500212224 > > And corresponding diff is below. > https://github.com/ryogrid/postgres/compare/0ec3c295e7594ed3af86bca1a4b4be269c2f069d...72329311a75630594bcaa38248255360b7e8e525 > > I explain about implementation of this patch. > > What is this patch > - add regression tests which test ecpg command notices such as warning > and errors > - test notices implemented in ecpg.addons file > > Basic policy on implementation > - do in a way that matches the method using the existing pg_regress > command as much as possible > - avoid methods that increase the scope of influence > > Next, I list answers to points that are likely to be pointed out in > advance below :) > - shell scripts and bat files is used due to ... > avoid non zero exit code of ecpg command makes tests failure > avoid increasing C code for executing binary which cares cross platform > - python code is used because I couldn't write meson.build > appropriately describe dependency about materials which is used on > tests without it. please help me... > - as you said, kick this kind of tests by pg_regress accompanied with > needless PG server process execution. but pg_regress doesn't execute > test without it and making pg_regress require modification which has > not small scope of influence Sorry, I re-send patch because a patch already sent includes needless stderr output file. NEW PATCH FILENAME: ecpg_cmd_notice_regress_test2.patch In addition, diff between patches does not affect test behavior. --- Sincerely, Ryo Kanbayashi https://github.com/ryogrid
Fwd: [PATCH] Add regression tests of ecpg command notice (error / warning)
On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi wrote: > > Hi hackers, > > When I wrote patch of ecpg command notice bug, I recognized needs of > regression tests for ecpg command notices and I say that I write the > tests. > > https://commitfest.postgresql.org/52/5497/ > https://www.postgresql.org/message-id/0efab1f6-5d8d-451f-a7dc-ef9c73ba9e02%40oss.nttdata.com > > This mail is about patch of the tests. > > Patch is attached to this mail : ecpg_cmd_notice_regress_test.patch > This patch passed CI. > https://cirrus-ci.com/build/4861827500212224 > > And corresponding diff is below. > https://github.com/ryogrid/postgres/compare/0ec3c295e7594ed3af86bca1a4b4be269c2f069d...72329311a75630594bcaa38248255360b7e8e525 > > I explain about implementation of this patch. > > What is this patch > - add regression tests which test ecpg command notices such as warning > and errors > - test notices implemented in ecpg.addons file > > Basic policy on implementation > - do in a way that matches the method using the existing pg_regress > command as much as possible > - avoid methods that increase the scope of influence > > Next, I list answers to points that are likely to be pointed out in > advance below :) > - shell scripts and bat files is used due to ... > avoid non zero exit code of ecpg command makes tests failure > avoid increasing C code for executing binary which cares cross platform > - python code is used because I couldn't write meson.build > appropriately describe dependency about materials which is used on > tests without it. please help me... > - as you said, kick this kind of tests by pg_regress accompanied with > needless PG server process execution. but pg_regress doesn't execute > test without it and making pg_regress require modification which has > not small scope of influence Sorry, I re-send patch because a patch already sent includes needless stderr output file. NEW PATCH FILENAME: ecpg_cmd_notice_regress_test2.patch In addition, diff between patches does not affect test behavior. --- Sincerely, Ryo Kanbayashi https://github.com/ryogrid ecpg_cmd_notice_regress_test2.patch Description: Binary data
[PATCH] Add regression tests of ecpg command notice (error / warning)
Hi hackers, When I wrote patch of ecpg command notice bug, I recognized needs of regression tests for ecpg command notices and I say that I write the tests. https://commitfest.postgresql.org/52/5497/ https://www.postgresql.org/message-id/0efab1f6-5d8d-451f-a7dc-ef9c73ba9e02%40oss.nttdata.com This mail is about patch of the tests. Patch is attached to this mail : ecpg_cmd_notice_regress_test.patch This patch passed CI. https://cirrus-ci.com/build/4861827500212224 And corresponding diff is below. https://github.com/ryogrid/postgres/compare/0ec3c295e7594ed3af86bca1a4b4be269c2f069d...72329311a75630594bcaa38248255360b7e8e525 I explain about implementation of this patch. What is this patch - add regression tests which test ecpg command notices such as warning and errors - test notices implemented in ecpg.addons file Basic policy on implementation - do in a way that matches the method using the existing pg_regress command as much as possible - avoid methods that increase the scope of influence Next, I list answers to points that are likely to be pointed out in advance below :) - shell scripts and bat files is used due to ... avoid non zero exit code of ecpg command makes tests failure avoid increasing C code for executing binary which cares cross platform - python code is used because I couldn't write meson.build appropriately describe dependency about materials which is used on tests without it. please help me... - as you said, kick this kind of tests by pg_regress accompanied with needless PG server process execution. but pg_regress doesn't execute test without it and making pg_regress require modification which has not small scope of influence --- Great regards, Ryo Kanbayashi https://github.com/ryogrid diff --git a/src/interfaces/ecpg/test/ecpg_schedule b/src/interfaces/ecpg/test/ecpg_schedule index 254a0bacc75..e1cc61d8c52 100644 --- a/src/interfaces/ecpg/test/ecpg_schedule +++ b/src/interfaces/ecpg/test/ecpg_schedule @@ -31,6 +31,8 @@ test: preproc/variable test: preproc/outofscope test: preproc/whenever test: preproc/whenever_do_continue +test: preproc/notice_check +test: preproc/notice_informix_check test: sql/array test: sql/binary test: sql/bytea diff --git a/src/interfaces/ecpg/test/expected/preproc-check_cmd_notice.stderr b/src/interfaces/ecpg/test/expected/preproc-check_cmd_notice.stderr new file mode 100644 index 000..31bfbc9f3bd --- /dev/null +++ b/src/interfaces/ecpg/test/expected/preproc-check_cmd_notice.stderr @@ -0,0 +1,13 @@ +preproc/notice.pgc:16: ERROR: AT option not allowed in CONNECT statement +preproc/notice.pgc:17: ERROR: AT option not allowed in DISCONNECT statement +preproc/notice.pgc:18: ERROR: AT option not allowed in SET CONNECTION statement +preproc/notice.pgc:21: WARNING: COPY FROM STDIN is not implemented +preproc/notice.pgc:25: ERROR: using variable "cursor_var" in different declare statements is not supported +preproc/notice.pgc:28: ERROR: cursor "duplicate_cursor" is already defined +preproc/notice.pgc:31: ERROR: SHOW ALL is not implemented +preproc/notice.pgc:33: ERROR: AT option not allowed in WHENEVER statement +preproc/notice.pgc:36: WARNING: no longer supported LIMIT #,# syntax passed to server +preproc/notice.pgc:39: WARNING: cursor "duplicate_cursor" has been declared but not opened +preproc/notice.pgc:39: WARNING: cursor "duplicate_cursor" has been declared but not opened +preproc/notice.pgc:39: WARNING: cursor ":cursor_var" has been declared but not opened +preproc/notice.pgc:39: WARNING: cursor ":cursor_var" has been declared but not opened diff --git a/src/interfaces/ecpg/test/expected/preproc-check_cmd_notice_informix.stderr b/src/interfaces/ecpg/test/expected/preproc-check_cmd_notice_informix.stderr new file mode 100644 index 000..0b9013c126e --- /dev/null +++ b/src/interfaces/ecpg/test/expected/preproc-check_cmd_notice_informix.stderr @@ -0,0 +1,2 @@ +preproc/notice_informix.pgc:11: ERROR: AT option not allowed in CLOSE DATABASE statement +preproc/notice_informix.pgc:14: ERROR: "database" cannot be used as cursor name in INFORMIX mode diff --git a/src/interfaces/ecpg/test/expected/preproc-notice_check.stderr b/src/interfaces/ecpg/test/expected/preproc-notice_check.stderr new file mode 100644 index 000..b31076a2527 --- /dev/null +++ b/src/interfaces/ecpg/test/expected/preproc-notice_check.stderr @@ -0,0 +1,15 @@ +notice.pgc:17: ERROR: AT option not allowed in CONNECT statement +notice.pgc:18: ERROR: AT option not allowed in DISCONNECT statement +notice.pgc:19: ERROR: AT option not allowed in SET CONNECTION statement +notice.pgc:20: ERROR: AT option not allowed in TYPE statement +notice.pgc:21: ERROR: AT option not allowed in WHENEVER statement +notice.pgc:22: ERROR: AT option not allowed in VAR statement +notice.pgc:25: WARNING: COPY FROM STDIN is not implemented +notice.pgc:29: ERROR: using variable "cursor_var" in differe
Re: typo in a comment of restrictinfo.c
{
"emoji": "😀",
"version": 1
}
typo in a comment of restrictinfo.c
Hi everyone, I found a typo in comment of restrictinfo.c line 99. ( https://github.com/postgres/postgres/blob/master/src/backend/optimizer/util/restrictinfo.c#L99 ) Not 'construcitng' but 'constructing' ? -- Best regards, Ryo Kanbayashi kanbayashi@gmail.com https://github.com/ryogrid diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c index 9e1458401c..ca3e764c20 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -96,7 +96,7 @@ make_restrictinfo(PlannerInfo *root, * make_plain_restrictinfo * * Common code for the main entry points and the recursive cases. Also, - * useful while contrucitng RestrictInfos above OR clause, which already has + * useful while constructing RestrictInfos above OR clause, which already has * RestrictInfos above its subclauses. */ RestrictInfo *
Re: ecpg command does not warn COPY ... FROM STDIN;
> On Thu, Jan 9, 2025 at 9:27 PM Ryo Kanbayashi > wrote: > > > On 2025/01/09 20:34, Ryo Kanbayashi wrote: > > > Dear Tom, Fujii-san, Kuroda-san, > > > > > > I saw comments of yours and recognized that better fix is below. > > > > > > - Fix of first attached patch which does not change warning message > > > > > > And I created patch entry of commitfest :) > > > https://commitfest.postgresql.org/52/5497/ > > > > > > What should I do additionally? > > > Do I need to write patches for current supporting versions? (12.x - 17.x) > > > > Testing the patch across all supported versions would be helpful. > > If adjustments are needed for specific versions, creating separate > > patches for those would also be appreciated. Since v12 is no longer > > supported, back-patching to it isn't necessary. > > thanks. > I try these :) > > > BTW, regarding the discussion on the list, please avoid top-posting; > > bottom-posting is the preferred style on this mailing list. > > I understand. > I'll be careful from now on :) > > (Please Ignore: I attach renamed patch file for updating patch file on > commitfest system) I wrote a patch for release v13 - v17 additionally and tested it for each release branch :) As a result, two patch is needed for this fix. copy_from_stdin_no_warning_for_master3.patch -> patch for master branch copy_from_stdin_no_warning_for_stables.patch -> patch for v13 - v17 check_patches.sh -> utility script for testing above two patches on each target branches patch_checking_note_cf.txt -> checking result by check_patches.sh and etc other files -> files which are needed for testing with check_patches.sh [commitfest entry] https://commitfest.postgresql.org/52/5497/ It would be helpful if someone could review patches I wrote :) -- Best regards, Ryo Kanbayashi https://github.com/ryogrid ryo@DESKTOP-IOASPN6:~/work/postgres$ uname -a Linux DESKTOP-IOASPN6 5.15.167.4-microsoft-standard-WSL2 #1 SMP Tue Nov 5 00:21:55 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux ryo@DESKTOP-IOASPN6:~/work/postgres$ gcc --version gcc (Ubuntu 10.5.0-1ubuntu1~20.04) 10.5.0 Copyright (C) 2020 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ryo@DESKTOP-IOASPN6:~/work/postgres$ bison --version bison (GNU Bison) 3.5.1 Written by Robert Corbett and Richard Stallman. Copyright (C) 2020 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ryo@DESKTOP-IOASPN6:~/work/postgres$ lex --version flex 2.6.4 ryo@DESKTOP-IOASPN6:~/work/postgres$ perl --version This is perl 5, version 30, subversion 0 (v5.30.0) built for x86_64-linux-gnu-thread-multi (with 60 registered patches, see perl -V for more detail) Copyright 1987-2019, Larry Wall Perl may be copied only under the terms of either the Artistic License or the GNU General Public License, which may be found in the Perl 5 source kit. Complete documentation for Perl, including FAQ lists, should be found on this system using "man perl" or "perldoc perl". If you have access to the Internet, point your browser at http://www.perl.org/, the Perl Home Page. ryo@DESKTOP-IOASPN6:~/work/postgres$ bash check_patches.sh -- Checking branch: master -- [HEAD] commit ca87c415e2fccf81cec6fd45698dde9fae0ab570 Author: Peter Eisentraut Date: Sat Jan 11 10:45:17 2025 +0100 Applying patch: ./copy_from_stdin_no_warning_for_master3.patch patching file src/interfaces/ecpg/preproc/ecpg.addons [Confirmation 1] Start building... Running: ./configure --enable-cassert --enable-tap-tests --prefix=/home/ryo/work/postgres/local_install >/dev/null 2>&1 Running: make -j 8 >/dev/null 2>&1 && make install >/dev/null 2>&1 [Confirmation 2] Running regression tests... [Confirmation 3] Checking output for command: ./local_install/bin/ecpg copy_from_should_be_warned.pgc [Confirmation 4] Checking no output for command: ./local_install/bin/ecpg copy_from_ok.pgc All checks passed on branch master. Cleaning up... Branch master checks complete. Moving on. -- Checking branch: REL_13_STABLE -- [HEAD] commit 84b8f6d9f59aa2d40ffb8edccb3f1eacff32b6c0 Author: Daniel Gustafsson Date: Fri Jan 10 22:02:58 2025 +0100 Applying patch: ./copy_from_stdin_no_warning_for_stables.patch patching file src/interfaces/ecpg/preproc/ecpg.addons
Re: ecpg command does not warn COPY ... FROM STDIN;
Kuroda-san, thank to your comment. > > I found a code validation bug in master branch. > > > > Now, ecpg does not support 'EXEC SQL COPY ... FROM STDIN ... ;' and > > code for warning it exits. > > > > https://github.com/postgres/postgres/blob/7b27f5fd36cb3270e8ac25aefd73b55 > > 2663d1392/src/interfaces/ecpg/preproc/ecpg.addons#L242-L245 > > --- > > ECPG: addon CopyStmt COPY opt_binary qualified_name opt_column_list > > copy_from opt_program copy_file_name copy_delimiter opt_with > > copy_options where_clause > > if (strcmp(@6, "from") == 0 && > > (strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0)) > > mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not > > implemented"); > > --- > > > > But it is not working. > > ecpg command fails to notice though code like above exits on pgc code. > > Good catch. I have a comment about the fix. > > The parser accepts a statement like "COPY ... FROM STDOUT", and ISTM it is > not either > implemented yet. However, the warning message only mentions STDIN case even > STDOUT > is specified. > > EXEC SQL COPY foo FROM STDOUT; > -> > WARNING: COPY FROM STDIN is not implemented > > I feel we can change like "COPY FROM STDIN/STDOUT...", or prepare two > messages. > Thought? I think your proposed fix is better too. So, I modified the patch. With new patch, warning message is changed like below :) ryo@DESKTOP-IOASPN6:~/work/postgres/src$ ../master/bin/ecpg copy_from_should_be_warned.pgc copy_from_should_be_warned.pgc:24: WARNING: COPY FROM STDIN/STDOUT is not implemented ryo@DESKTOP-IOASPN6:~/work/postgres/src$ -- Best regards, Ryo Kanbayashi https://github.com/ryogrid On Wed, Jan 8, 2025 at 10:35 PM Hayato Kuroda (Fujitsu) wrote: > > Dear Kanbayashi-san, > > > I found a code validation bug in master branch. > > > > Now, ecpg does not support 'EXEC SQL COPY ... FROM STDIN ... ;' and > > code for warning it exits. > > > > https://github.com/postgres/postgres/blob/7b27f5fd36cb3270e8ac25aefd73b55 > > 2663d1392/src/interfaces/ecpg/preproc/ecpg.addons#L242-L245 > > --- > > ECPG: addon CopyStmt COPY opt_binary qualified_name opt_column_list > > copy_from opt_program copy_file_name copy_delimiter opt_with > > copy_options where_clause > > if (strcmp(@6, "from") == 0 && > > (strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0)) > > mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not > > implemented"); > > --- > > > > But it is not working. > > ecpg command fails to notice though code like above exits on pgc code. > > Good catch. I have a comment about the fix. > > The parser accepts a statement like "COPY ... FROM STDOUT", and ISTM it is > not either > implemented yet. However, the warning message only mentions STDIN case even > STDOUT > is specified. > > EXEC SQL COPY foo FROM STDOUT; > -> > WARNING: COPY FROM STDIN is not implemented > > I feel we can change like "COPY FROM STDIN/STDOUT...", or prepare two > messages. > Thought? > > Best regards, > Hayato Kuroda > FUJITSU LIMITED > copy_from_stdin_no_warning2.patch Description: Binary data
Re: ecpg command does not warn COPY ... FROM STDIN;
On Thu, Jan 9, 2025 at 9:27 PM Ryo Kanbayashi wrote: > > > On 2025/01/09 20:34, Ryo Kanbayashi wrote: > > > Dear Tom, Fujii-san, Kuroda-san, > > > > > > I saw comments of yours and recognized that better fix is below. > > > > > > - Fix of first attached patch which does not change warning message > > > > > > And I created patch entry of commitfest :) > > > https://commitfest.postgresql.org/52/5497/ > > > > > > What should I do additionally? > > > Do I need to write patches for current supporting versions? (12.x - 17.x) > > > > Testing the patch across all supported versions would be helpful. > > If adjustments are needed for specific versions, creating separate > > patches for those would also be appreciated. Since v12 is no longer > > supported, back-patching to it isn't necessary. > > thanks. > I try these :) > > > BTW, regarding the discussion on the list, please avoid top-posting; > > bottom-posting is the preferred style on this mailing list. > > I understand. > I'll be careful from now on :) > > (Please Ignore: I attach renamed patch file for updating patch file on > commitfest system) PLEASE IGNORE THIS MAIL (I attached wrong format patch file. So re-attach modified one for CFBot) --- Great Reagrds, Ryo Kanbayashi copy_from_stdin_no_warning_for_master2.patch Description: Binary data
Re: ecpg command does not warn COPY ... FROM STDIN;
On Sun, Jan 12, 2025 at 12:56 PM Fujii Masao wrote: > > > > On 2025/01/12 2:04, Ryo Kanbayashi wrote: > > I wrote a patch for release v13 - v17 additionally and tested it for > > each release branch :) > > As a result, two patch is needed for this fix. > > Thanks for the patches! Barring any objections, > I plan to commit them with the following commit log. > > --- > ecpg: Restore detection of unsupported COPY FROM STDIN. > > The ecpg command includes code to warn about unsupported COPY FROM STDIN > statements in input files. However, since commit 3d009e45bd, > this functionality has been broken due to a bug introduced in that commit, > causing ecpg to fail to detect the statement. > > This commit resolves the issue, restoring ecpg's ability to detect > COPY FROM STDIN and issue a warning as intended. > > Back-patch to all supported versions. > > Author: Ryo Kanbayashi > Reviewed-by: Hayato Kuroda, Tom Lane > Discussion: > https://postgr.es/m/canon0ez_t5udcuev8c1yormisjiu5wu681eevzzgkwoeikh...@mail.gmail.com > --- Thank you for reviewing patch :) The commit log matches with my recognition and has no problem. > > check_patches.sh -> utility script for testing above two patches on > > each target branches > > Should we add a regression test to ensure ecpg correctly reports errors > and warnings, including the warning for COPY FROM STDIN? This could help > catch similar bugs more effectively. If agreed, we could create this > as a separate patch. Of course there is no problem! I think a test like above becomes a good regression test too. -- Best regards, Ryo Kanbayashi https://github.com/ryogrid
Re: ecpg command does not warn COPY ... FROM STDIN;
On Fri, Jan 10, 2025 at 5:45 PM Ryo Kanbayashi wrote: > > On Thu, Jan 9, 2025 at 9:27 PM Ryo Kanbayashi > wrote: > > > > > On 2025/01/09 20:34, Ryo Kanbayashi wrote: > > > > Dear Tom, Fujii-san, Kuroda-san, > > > > > > > > I saw comments of yours and recognized that better fix is below. > > > > > > > > - Fix of first attached patch which does not change warning message > > > > > > > > And I created patch entry of commitfest :) > > > > https://commitfest.postgresql.org/52/5497/ > > > > > > > > What should I do additionally? > > > > Do I need to write patches for current supporting versions? (12.x - > > > > 17.x) > > > > > > Testing the patch across all supported versions would be helpful. > > > If adjustments are needed for specific versions, creating separate > > > patches for those would also be appreciated. Since v12 is no longer > > > supported, back-patching to it isn't necessary. > > > > thanks. > > I try these :) > > > > > BTW, regarding the discussion on the list, please avoid top-posting; > > > bottom-posting is the preferred style on this mailing list. > > > > I understand. > > I'll be careful from now on :) PLEASE IGNORE THIS MAIL SORRY TO BOTHER YOU AGAIN... (I have not attached correct format patch file yet. So re-attach modified one for CFBot) Great Reagrds, Ryo Kanbayashi copy_from_stdin_no_warning_for_master3.patch Description: Binary data
Re: ecpg command does not warn COPY ... FROM STDIN;
On Wed, Jan 15, 2025 at 1:34 AM Fujii Masao wrote: > > > > On 2025/01/12 18:27, Ryo Kanbayashi wrote: > > Thank you for reviewing patch :) > > The commit log matches with my recognition and has no problem. > > Pushed. Thanks! > > >>> check_patches.sh -> utility script for testing above two patches on > >>> each target branches > >> > >> Should we add a regression test to ensure ecpg correctly reports errors > >> and warnings, including the warning for COPY FROM STDIN? This could help > >> catch similar bugs more effectively. If agreed, we could create this > >> as a separate patch. > > > > Of course there is no problem! > > I think a test like above becomes a good regression test too. > > So, will you give creating the patch a try? Yes. I try to write the patch for regression test of ecpg command warning and error notice :) BTW, How should we handle commit fest entry below? "ecpg command does not warn COPY ... FROM STDIN;" https://commitfest.postgresql.org/52/5497/ I think that the patch of regression test is not included the entry. --- Great regards, Ryo Kanbayashi https://github.com/ryogrid
Re: ecpg command does not warn COPY ... FROM STDIN;
{
"emoji": "🙏",
"version": 1
}
ecpg command does not warn COPY ... FROM STDIN;
Hi hackers,
I found a code validation bug in master branch.
Now, ecpg does not support 'EXEC SQL COPY ... FROM STDIN ... ;' and
code for warning it exits.
https://github.com/postgres/postgres/blob/7b27f5fd36cb3270e8ac25aefd73b552663d1392/src/interfaces/ecpg/preproc/ecpg.addons#L242-L245
---
ECPG: addon CopyStmt COPY opt_binary qualified_name opt_column_list
copy_from opt_program copy_file_name copy_delimiter opt_with
copy_options where_clause
if (strcmp(@6, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
---
But it is not working.
ecpg command fails to notice though code like above exits on pgc code.
# COPY ... FROM STDIN code
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ cat copy_from_should_be_warned.pgc
#include
#include
EXEC SQL WHENEVER SQLERROR CALL die();
EXEC SQL WHENEVER NOT FOUND DO BREAK;
void
die(void)
{
fprintf(stderr, "%s\n", sqlca.sqlerrm.sqlerrmc);
exit(1);
}
int
main(void)
{
EXEC SQL BEGIN DECLARE SECTION;
const char *target = "postgres@localhost:5432";
const char *user = "ryo";
const char *passwd = "";
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :target USER :user USING :passwd;
EXEC SQL COPY name_age_list FROM STDIN;
EXEC SQL COMMIT;
EXEC SQL DISCONNECT ALL;
return 0;
}
-
I executed ecpg command for above code.
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ ../master/bin/ecpg
copy_from_should_be_warned.pgc
ryo@DESKTOP-IOASPN6:~/work/postgres/src$
But there was no warning and generaged c source.
So, I wrote patch.
the patch changes @6 on code above to @5.
Checked variable was wrong.
After apply patch, warning is shown.
(c source is generated as before)
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ ../master/bin/ecpg
copy_from_should_be_warned.pgc
copy_from_should_be_warned.pgc:24: WARNING: COPY FROM STDIN is not implemented
ryo@DESKTOP-IOASPN6:~/work/postgres/src$
--
Best regards,
Ryo Kanbayashi
kanbayashi@gmail.com
https://github.com/ryogrid
ryo@DESKTOP-IOASPN6:~/work/postgres$ git log
commit 7b27f5fd36cb3270e8ac25aefd73b552663d1392 (HEAD -> master, origin/master,
origin/HEAD)
Author: Peter Eisentraut
Date: Wed Jan 8 09:20:01 2025 +0100
plpgsql: pure parser and reentrant scanner
ryo@DESKTOP-IOASPN6:~/work/postgres$ uname -a
Linux DESKTOP-IOASPN6 5.15.167.4-microsoft-standard-WSL2 #1 SMP Tue Nov 5
00:21:55 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
ryo@DESKTOP-IOASPN6:~/work/postgres$ gcc --version
gcc (Ubuntu 10.5.0-1ubuntu1~20.04) 10.5.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
ryo@DESKTOP-IOASPN6:~/work/postgres$ bison --version
bison (GNU Bison) 3.5.1
Written by Robert Corbett and Richard Stallman.
Copyright (C) 2020 Free
Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
ryo@DESKTOP-IOASPN6:~/work/postgres$ lex --version
flex 2.6.4
# local postgres install dir
ryo@DESKTOP-IOASPN6:~/work/postgres$ ls master/
bin data include lib share
# COPY ... FROM STDIN code
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ cat copy_from_should_be_warned.pgc
#include
#include
EXEC SQL WHENEVER SQLERROR CALL die();
EXEC SQL WHENEVER NOT FOUND DO BREAK;
void
die(void)
{
fprintf(stderr, "%s\n", sqlca.sqlerrm.sqlerrmc);
exit(1);
}
int
main(void)
{
EXEC SQL BEGIN DECLARE SECTION;
const char *target = "postgres@localhost:5432";
const char *user = "ryo";
const char *passwd = "";
EXEC SQL END DECLARE SECTION;
EXEC SQL CONNECT TO :target USER :user USING :passwd;
EXEC SQL COPY name_age_list FROM STDIN;
EXEC SQL COMMIT;
EXEC SQL DISCONNECT ALL;
return 0;
}
# before apply patch -> No Warning
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ ../master/bin/ecpg
copy_from_should_be_warned.pgc
ryo@DESKTOP-IOASPN6:~/work/postgres/src$
# after apply patch -> Warning is shown
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ ../master/bin/ecpg
copy_from_should_be_warned.pgc
copy_from_should_be_warned.pgc:24: WARNING: COPY FROM STDIN is not implemented
ryo@DESKTOP-IOASPN6:~/work/postgres/src$
# COPY ... FROM filename code
ryo@DESKTOP-IOASPN6:~/work/postgres/src$ cat copy_from_ok.pgc
#include
#include
EXEC SQL WHENEVER SQLERROR CALL die();
EXEC SQL WHENEVER NOT FOUND DO BREAK;
void
die(void)
{
fprintf(stderr, "%s\n", sqlca.sqlerrm.sqlerrmc);
exit(1);
}
int
main(void)
{
EXEC SQL BEGIN DEC
Re: ecpg command does not warn COPY ... FROM STDIN;
> On 2025/01/09 20:34, Ryo Kanbayashi wrote: > > Dear Tom, Fujii-san, Kuroda-san, > > > > I saw comments of yours and recognized that better fix is below. > > > > - Fix of first attached patch which does not change warning message > > > > And I created patch entry of commitfest :) > > https://commitfest.postgresql.org/52/5497/ > > > > What should I do additionally? > > Do I need to write patches for current supporting versions? (12.x - 17.x) > > Testing the patch across all supported versions would be helpful. > If adjustments are needed for specific versions, creating separate > patches for those would also be appreciated. Since v12 is no longer > supported, back-patching to it isn't necessary. thanks. I try these :) > BTW, regarding the discussion on the list, please avoid top-posting; > bottom-posting is the preferred style on this mailing list. I understand. I'll be careful from now on :) (Please Ignore: I attach renamed patch file for updating patch file on commitfest system) -- Best regards, Ryo Kanbayashi https://github.com/ryogrid copy_from_stdin_no_warning_for_master.patch Description: Binary data
Re: ecpg command does not warn COPY ... FROM STDIN;
Dear Tom, Fujii-san, Kuroda-san, I saw comments of yours and recognized that better fix is below. - Fix of first attached patch which does not change warning message And I created patch entry of commitfest :) https://commitfest.postgresql.org/52/5497/ What should I do additionally? Do I need to write patches for current supporting versions? (12.x - 17.x) -- Best regards, Ryo Kanbayashi https://github.com/ryogrid On Thu, Jan 9, 2025 at 4:53 PM Hayato Kuroda (Fujitsu) wrote: > > Dear Tom, Fujii-san, > > > > ISTM that ecpg supports COPY TO STDOUT and includes the regression test > > "copystdout" for it. No? > > > > Oh right. (Pokes at it...) It looks like the backend accepts > > "FROM STDOUT" as a synonym for "FROM STDIN", so that's why this > > is checking for both spellings. But I agree it'd be better > > for the error message to only use the standard spelling. > > > > Also I tried > > > > regression=# copy int4_tbl from program stdin; > > ERROR: STDIN/STDOUT not allowed with PROGRAM > > > > So we probably don't need to bother with adjusting the check > > to allow that. > > To confirm - I have no objections for the decision. I'm also think it is not > an usual grammar. I checked the doc [1] and I could not find "COPY FROM > STDOUT". > I.e., first version is enough. > > [1]: https://www.postgresql.org/docs/devel/sql-copy.html > > Best regards, > Hayato Kuroda > FUJITSU LIMITED > copy_from_stdin_no_warning.patch Description: Binary data
Re: [PATCH] Add regression tests of ecpg command notice (error / warning)
On Thu, Feb 13, 2025 at 10:49 PM Fujii Masao wrote: > > > > On 2025/02/06 8:57, Ryo Kanbayashi wrote: > > On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi > > wrote: > >> > >> Hi hackers, > >> > >> When I wrote patch of ecpg command notice bug, I recognized needs of > >> regression tests for ecpg command notices and I say that I write the > >> tests. > > Thanks for working on this! > > > >> I explain about implementation of this patch. > >> > >> What is this patch > >> - add regression tests which test ecpg command notices such as warning > >> and errors > >> - test notices implemented in ecpg.addons file > >> > >> Basic policy on implementation > >> - do in a way that matches the method using the existing pg_regress > >> command as much as possible > >> - avoid methods that increase the scope of influence > >> > >> Next, I list answers to points that are likely to be pointed out in > >> advance below :) > >> - shell scripts and bat files is used due to ... > >> avoid non zero exit code of ecpg command makes tests failure > >> avoid increasing C code for executing binary which cares cross > >> platform > >> - python code is used because I couldn't write meson.build > >> appropriately describe dependency about materials which is used on > >> tests without it. please help me... > >> - as you said, kick this kind of tests by pg_regress accompanied with > >> needless PG server process execution. but pg_regress doesn't execute > >> test without it and making pg_regress require modification which has > >> not small scope of influence > > Wouldn't it be simpler to use the existing TAP test mechanism, > as shown in the attached patch? Please note that this patch is very WIP, > so there would be many places that need further implementation and refinement. Fujii San, Thank you for reviewing and indication of better implementation. I rewrite my patch based on your reference implementation :) --- Great regards, Ryo Kanbayashi NTT Open Source Software Center
Re: [PATCH] Add regression tests of ecpg command notice (error / warning)
>On Mon, Mar 3, 2025 at 12:23 PM Fujii Masao
>wrote:
> On 2025/03/01 19:45, Ryo Kanbayashi wrote:
> >> +program_help_ok('ecpg');
> >> +program_version_ok('ecpg');
> >> +program_options_handling_ok('ecpg');
> >> +command_fails(['ecpg'], 'ecpg without arguments fails');
> >>
> >> These checks seem unnecessary in 002 since they're already covered in 001.
> >
> > I reflected above.
>
> Thanks for updating the patch!
>
> I've made some minor fixes and cosmetic adjustments.
> The updated patch is attached.
>
> Unless there are any objections, I'll commit it.
Thanks for reviewing and adustments.
There is no objections :)
---
Great regards,
Ryo Kanbayashi
NTT Open Source Software Center
Re: [PATCH] Add regression tests of ecpg command notice (error / warning)
On Tue, Feb 18, 2025 at 12:49 PM Ryo Kanbayashi wrote: > > On Thu, Feb 13, 2025 at 10:49 PM Fujii Masao > wrote: > > > > > > > > On 2025/02/06 8:57, Ryo Kanbayashi wrote: > > > On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi > > > wrote: > > >> > > >> Hi hackers, > > >> > > >> When I wrote patch of ecpg command notice bug, I recognized needs of > > >> regression tests for ecpg command notices and I say that I write the > > >> tests. > > > > Thanks for working on this! > > > > > > >> I explain about implementation of this patch. > > >> > > >> What is this patch > > >> - add regression tests which test ecpg command notices such as warning > > >> and errors > > >> - test notices implemented in ecpg.addons file > > >> > > >> Basic policy on implementation > > >> - do in a way that matches the method using the existing pg_regress > > >> command as much as possible > > >> - avoid methods that increase the scope of influence > > >> > > >> Next, I list answers to points that are likely to be pointed out in > > >> advance below :) > > >> - shell scripts and bat files is used due to ... > > >> avoid non zero exit code of ecpg command makes tests failure > > >> avoid increasing C code for executing binary which cares cross > > >> platform > > >> - python code is used because I couldn't write meson.build > > >> appropriately describe dependency about materials which is used on > > >> tests without it. please help me... > > >> - as you said, kick this kind of tests by pg_regress accompanied with > > >> needless PG server process execution. but pg_regress doesn't execute > > >> test without it and making pg_regress require modification which has > > >> not small scope of influence > > > > Wouldn't it be simpler to use the existing TAP test mechanism, > > as shown in the attached patch? Please note that this patch is very WIP, > > so there would be many places that need further implementation and > > refinement. > > Fujii San, > > Thank you for reviewing and indication of better implementation. > I rewrite my patch based on your reference implementation :) Fujii San and other hackers, I have rewrote my patch on TAP test sttyle :) File for build are also updated. Commitfest entry: https://commitfest.postgresql.org/patch/5543/ --- Great regards, Ryo Kanbayashi NTT Open Source Software Center ecpg-notice-regress-patch-tap-ver.patch Description: Binary data
Re: [PATCH] Add regression tests of ecpg command notice (error / warning)
On Fri, Feb 28, 2025 at 11:27 PM Fujii Masao
wrote:
> On 2025/02/28 9:24, Ryo Kanbayashi wrote:
> > I have rewrote my patch on TAP test sttyle :)
> > File for build are also updated.
>
> Thanks for updating the patch!
Thanks for review:)
>
> +'tests': [
> + 't/001_ecpg_notice.pl',
> + 't/002_ecpg_notice_informix.pl',
>
> Since neither test emits "notice" messages, shouldn't the test file
> names be revised to reflect this?
I replaced "notice" to "err_warn_msg"
> Also, I'm unsure if it's ideal to place input files directly under
> the "t" directory. I looked for similar TAP tests with input files,
> but I couldn't find any examples to guide this decision...
I couldn't too. So places are not changed.
> +program_help_ok('ecpg');
> +program_version_ok('ecpg');
> +program_options_handling_ok('ecpg');
> +command_fails(['ecpg'], 'ecpg without arguments fails');
>
> These checks seem unnecessary in 002 since they're already covered in 001.
I reflected above.
---
Great regards,
Ryo Kanbayashi
ecpg-notice-regress-patch-tap-ver-rebased.patch
Description: Binary data
Re: PGSERVICEFILE as part of a normal connection string
On Mon, Jan 27, 2025 at 2:01 PM Michael Paquier wrote: > > On Wed, Nov 20, 2024 at 02:58:43AM -0500, Corey Huinker wrote: > > Currently, a lot of our utility scripts (anything that uses > > connectDatabase) don't support service=name params or PGSERVICE=name env > > vars, which is really too bad. I previously thought that this was because > > of a lack of interest, but perhaps people do want it? > > I'm all for more test coverage, FWIW. > > Torsten, the patch has been waiting on input from you based on my > latest review for some time, so I have marked it as returned with > feedback in the CP app. Feel free to resubmit a new version if you > are planning to work on that. TO: Torsten, CC: Micael and other hackers If you can't work for ther patch for a while because you are busy or other some reason, I can become additinal reviewer and apply review comments from Micael to the patch instead of you. If you don't want my action, please reply and notice me that. If possible, within a week :) Just to let you know, my action is not intended to steal your contribution but to prevent your good idea from being lost. TO: Mecael and other hackers, There are any problem in light of community customs? --- Great regards, Ryo Kanbayashi
Re: PGSERVICEFILE as part of a normal connection string
On Thu, Mar 13, 2025 at 9:42 AM Michael Paquier wrote: > > On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote: > > If you can't work for ther patch for a while because you are busy or > > other some reason, > > I can become additinal reviewer and apply review comments from Micael > > to the patch instead of you. > > > > If you don't want my action, please reply and notice me that. If > > possible, within a week :) > > Putting a bit of context here. Most of the Postgres hackers based in > Japan had a meeting last Friday, and Kanbayashi-san has asked me about > patches that introduce to simpler code paths in the tree that could be > worked on for this release. I've mentioned this thread to him. > > > Just to let you know, my action is not intended to steal your > > contribution but to prevent your good idea from being lost. > > Authors and reviewers get busy because of life and work matters, and > contributions are listed in the commit logs for everybody who > participates. If you can help move this patch forward, thanks a lot > for the help! IMO, that would be great. The patch set still needs > more reorganization and adjustments, but I think that we can get it > there. On Thu, Mar 13, 2025 at 3:07 PM Laurenz Albe wrote: > > On Thu, 2025-03-13 at 08:53 +0900, Ryo Kanbayashi wrote: > > Just to let you know, my action is not intended to steal your > > contribution but to prevent your good idea from being lost. > > > > TO: Mecael and other hackers, > > > > There are any problem in light of community customs? > > Anything submitted to the mailing list is no longer private > intellectual property. You are free and welcome to start working > on any patch that you are interested in and that seems neglected > by the author. There is no problem with listing more than one > author. Michael and Laurenz, Thank you for context description and comments to my action :) I start coding to complete the patch :) --- Great regards, Ryo Kanbayashi
Re: PGSERVICEFILE as part of a normal connection string
On Mon, Jan 27, 2025 at 2:01 PM Michael Paquier wrote: > On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote: > > Putting a bit of context here. Most of the Postgres hackers based in > > Japan had a meeting last Friday, and Kanbayashi-san has asked me about > > patches that introduce to simpler code paths in the tree that could be > > worked on for this release. I've mentioned this thread to him. > > > > > Just to let you know, my action is not intended to steal your > > > contribution but to prevent your good idea from being lost. > > > > Authors and reviewers get busy because of life and work matters, and > > contributions are listed in the commit logs for everybody who > > participates. If you can help move this patch forward, thanks a lot > > for the help! IMO, that would be great. The patch set still needs > > more reorganization and adjustments, but I think that we can get it > > there Michael, CC: Torsten I reviewed the patch and add some modification described below. part of https://www.postgresql.org/message-id/Zz2AE7NKKLIZTtEh%40paquier.xyz > +# This tests "service" and "servicefile" > > You are introducing tests for the existing "service", as well as tests > for the new "servicefile". Could it be possible to split that into > two patches for clarity? You'd want one to provide coverage for the > existing features (PGSERVICEFILE, PGSERVICE and connection parameter > "service"), then add tests for the new feature "servicename" with its > libpq implementation. That would make your main patch simpler, as > well. > > +open my $fh, '>', $srvfile or die $!; > +print $fh "[my_srv]\n"; > +print $fh +($node->connstr =~ s/ /\n/gr), "\n"; > +close $fh; > > Sure that's OK on Windows where we have CRLFs, not just LFs? I did... * Split the patch to two patches 1) regression test of existing features. 2) adding servicefile option feature, its regression test and etc * Add codes which care new line code of Windows * Add comments and apply formatter :) --- Great Regards, Ryo Kanbayashi v1-0001-add-regression-test-of-service-option.patch Description: Binary data v2-0001-PGSERVICEFILE-as-part-of-the-connection-string.patch Description: Binary data
[PATCH] PGSERVICEFILE as part of a normal connection string
On Thu, Mar 20, 2025 at 5:39 PM Ryo Kanbayashi wrote: > > On Mon, Jan 27, 2025 at 2:01 PM Michael Paquier wrote: > > On Thu, Mar 13, 2025 at 08:53:49AM +0900, Ryo Kanbayashi wrote: > > > Putting a bit of context here. Most of the Postgres hackers based in > > > Japan had a meeting last Friday, and Kanbayashi-san has asked me about > > > patches that introduce to simpler code paths in the tree that could be > > > worked on for this release. I've mentioned this thread to him. > > > > > > > Just to let you know, my action is not intended to steal your > > > > contribution but to prevent your good idea from being lost. > > > > > > Authors and reviewers get busy because of life and work matters, and > > > contributions are listed in the commit logs for everybody who > > > participates. If you can help move this patch forward, thanks a lot > > > for the help! IMO, that would be great. The patch set still needs > > > more reorganization and adjustments, but I think that we can get it > > > there > > Michael, > CC: Torsten > > I reviewed the patch and add some modification described below. > > part of https://www.postgresql.org/message-id/Zz2AE7NKKLIZTtEh%40paquier.xyz > > +# This tests "service" and "servicefile" > > > > You are introducing tests for the existing "service", as well as tests > > for the new "servicefile". Could it be possible to split that into > > two patches for clarity? You'd want one to provide coverage for the > > existing features (PGSERVICEFILE, PGSERVICE and connection parameter > > "service"), then add tests for the new feature "servicename" with its > > libpq implementation. That would make your main patch simpler, as > > well. > > > > +open my $fh, '>', $srvfile or die $!; > > +print $fh "[my_srv]\n"; > > +print $fh +($node->connstr =~ s/ /\n/gr), "\n"; > > +close $fh; > > > > Sure that's OK on Windows where we have CRLFs, not just LFs? > > I did... > * Split the patch to two patches >1) regression test of existing features. >2) adding servicefile option feature, its regression test and etc > * Add codes which care new line code of Windows > * Add comments and apply formatter :) Sorry, I found a miss on 006_service.pl. Fixed patch is attached... --- Great Regards, Ryo Kanbayashi v2-0001-add-regression-test-of-service-option.patch Description: Binary data
Re: [PATCH] PGSERVICEFILE as part of a normal connection string
On Fri, Mar 28, 2025 at 10:44 AM Michael Paquier wrote: > > On Thu, Mar 27, 2025 at 06:31:14PM +0900, Michael Paquier wrote: > > With all that in mind and more documentation added to the test, I've > > applied 0001, so let's see what the buildfarm has to say. The CI was > > stable, so it's a start. > > The buildfarm (particularly the Windows members that worried me), have > reported back and I am not seeing any failures, so we should be good > with 72c2f36d5727. Thank you for review and additional modification to the patch. I'm glad the patch was made in time for this release, even if it was just a partial one. On Fri, Mar 28, 2025 at 8:57 AM Michael Paquier wrote: > > I am not sure that I'll have the time to look at 0002 for this release > > cycle, could it be possible to get a rebase for it? > Here is a simple rebase that I have been able to assemble this > morning. I won't have the space to review it for this release cycle > unfortunately, but at least it works in the CI. I'm sorry I couldn't respond to your request :( > I am moving this patch entry to the next CF for v19, as a result of > that. OK Thanks :) On Thu, Mar 27, 2025 at 6:31 PM Michael Paquier wrote: > I have spent quite a bit of time on the review 0001 with the new > tests to get something in for this release, and there was quite a bit > going on there: > - The script should set PGSYSCONFDIR, or it could grab data that > depend on the host. This can use the temporary folder created in the > test. > - On the same ground, we need a similar tweak for PGSERVICEFILE or we > would go into pqGetHomeDirectory() and look at a HOME folder (WIN32 > and non-WIN32). > > With that addressed, there could be much more tests, like for cases > where PGSERVICEFILE is set but points to a file that does not exist, > more combinations between URIs, connection parameters and PGSERVICE, > for success and failure cases, empty service file, etc. > > Another thing that I've noticed to be useful to cover is the case > based on the hardcoded service file name pg_service.conf in > PGSYSCONFDIR, which is used as a fallback in the code if the service > name cannot be found in the initial PGSERVICEFILE, acting as a > fallback option. As long as PGSYSCONFDIR is set, we could test one in > isolation using the temporary folder created by the test. I check and modify 0002 patch (adding servicefile option and its regression tests) in light of the above and committed 0001 patch (regression test of existing features) toward next release :) --- Great Regards, Ryo Kanbayashi
Re: [PATCH] PGSERVICEFILE as part of a normal connection string
On Sat, Mar 22, 2025 at 4:46 PM Michael Paquier wrote:
>
> On Thu, Mar 20, 2025 at 06:16:44PM +0900, Ryo Kanbayashi wrote:
> > Sorry, I found a miss on 006_service.pl.
> > Fixed patch is attached...
>
> Please note that the commit fest app needs all the patches of a a set
> to be posted in the same message. In this case, v2-0001 is not going
> to get automatic test coverage.
>
> Your patch naming policy is also a bit confusing. I would suggest to
> use `git format-patch -vN -2`, where N is your version number. 0001
> would be the new tests for service files, and 0002 the new feature,
> with its own tests.
All right.
I attached patches generated with your suggested command :)
> +if ($windows_os) {
> +
> +# Windows: use CRLF
> +print $fh "[my_srv]", "\r\n";
> +print $fh join( "\r\n", split( ' ', $node->connstr ) ), "\r\n";
> +}
> +else {
> +# Non-Windows: use LF
> +print $fh "[my_srv]", "\n";
> +print $fh join( "\n", split( ' ', $node->connstr ) ), "\n";
> +}
> +close $fh;
>
> That's duplicated. Let's perhaps use a $newline variable and print
> into the file using the $newline?
OK.
I reflected above comment.
> Question: you are doing things this way in the test because fgets() is
> what is used by libpq to retrieve the lines of the service file, is
> that right?
No. I'm doing above way simply because line ending code of service file
wrote by users may become CRLF in Windows platform.
> Please note that the CI is failing. It seems to me that you are
> missing a done_testing() at the end of the script. If you have a
> github account, I'd suggest to set up a CI in your own fork of
> Postgres, this is really helpful to double-check the correctness of a
> patch before posting it to the lists, and saves in round trips between
> author and reviewer. Please see src/tools/ci/README in the code tree
> for details.
Sorry.
I'm using Cirrus CI with GitHub and I checked passing the CI.
But there were misses when I created patch files...
> +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
>
> These dates are incorrect. Should be 2025, as it's a new file.
OK.
> +++ b/src/interfaces/libpq/t/007_servicefile_opt.pl
> @@ -0,0 +1,100 @@
> +# Copyright (c) 2023-2024, PostgreSQL Global Development Group
>
> Incorrect date again in the second path with the new feature. I'd
> suggest to merge all the tests in a single script, with only one node
> initialized and started.
OK.
Additional test scripts have been merged to a single script ^^ b
---
Great regards,
Ryo Kanbayashi
From 69c4f4eb8e0ed40c434867fbb740a68383623da9 Mon Sep 17 00:00:00 2001
From: Ryo Kanbayashi
Date: Sun, 23 Mar 2025 11:41:06 +0900
Subject: [PATCH v3 1/2] add regression test of service connection option.
---
src/interfaces/libpq/meson.build | 1 +
src/interfaces/libpq/t/006_service.pl | 79 +++
2 files changed, 80 insertions(+)
create mode 100644 src/interfaces/libpq/t/006_service.pl
diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build
index 19f4a52a97a..292fecf3320 100644
--- a/src/interfaces/libpq/meson.build
+++ b/src/interfaces/libpq/meson.build
@@ -122,6 +122,7 @@ tests += {
't/003_load_balance_host_list.pl',
't/004_load_balance_dns.pl',
't/005_negotiate_encryption.pl',
+ 't/006_service.pl',
],
'env': {
'with_ssl': ssl_library,
diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl
new file mode 100644
index 000..045e25a05d3
--- /dev/null
+++ b/src/interfaces/libpq/t/006_service.pl
@@ -0,0 +1,79 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+use strict;
+use warnings FATAL => 'all';
+use Config;
+use PostgreSQL::Test::Utils;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+# This tests "service" connection options.
+
+# Cluster setup which is shared for testing both load balancing methods
+my $node = PostgreSQL::Test::Cluster->new('node');
+
+# Create a data directory with initdb
+$node->init();
+
+# Start the PostgreSQL server
+$node->start();
+
+my $td = PostgreSQL::Test::Utils::tempdir;
+my $srvfile = "$td/pgsrv.conf";
+
+# Windows: use CRLF
+# Non-Windows: use LF
+my $newline = $windows_os ? "\r\n" : "\n";
+
+# Create a service file
+open my $fh, '>', $srvfile or die $!;
+print $fh "[my_srv]", $newline;
+print $fh join( $newline, split( ' ', $node->connstr ) ), $newline;
+
+
Re: [PATCH] PGSERVICEFILE as part of a normal connection string
On Sat, Mar 29, 2025 at 3:35 PM Ryo Kanbayashi wrote: > On Fri, Mar 28, 2025 at 8:57 AM Michael Paquier wrote: > > > I am not sure that I'll have the time to look at 0002 for this release > > > cycle, could it be possible to get a rebase for it? > > Here is a simple rebase that I have been able to assemble this > > morning. I won't have the space to review it for this release cycle > > unfortunately, but at least it works in the CI. > > I'm sorry I couldn't respond to your request :( > > > I am moving this patch entry to the next CF for v19, as a result of > > that. > > OK > Thanks :) > > On Thu, Mar 27, 2025 at 6:31 PM Michael Paquier wrote: > > I have spent quite a bit of time on the review 0001 with the new > > tests to get something in for this release, and there was quite a bit > > going on there: > > - The script should set PGSYSCONFDIR, or it could grab data that > > depend on the host. This can use the temporary folder created in the > > test. > > - On the same ground, we need a similar tweak for PGSERVICEFILE or we > > would go into pqGetHomeDirectory() and look at a HOME folder (WIN32 > > and non-WIN32). > > > > With that addressed, there could be much more tests, like for cases > > where PGSERVICEFILE is set but points to a file that does not exist, > > more combinations between URIs, connection parameters and PGSERVICE, > > for success and failure cases, empty service file, etc. > > > > Another thing that I've noticed to be useful to cover is the case > > based on the hardcoded service file name pg_service.conf in > > PGSYSCONFDIR, which is used as a fallback in the code if the service > > name cannot be found in the initial PGSERVICEFILE, acting as a > > fallback option. As long as PGSYSCONFDIR is set, we could test one in > > isolation using the temporary folder created by the test. > > I check and modify 0002 patch (adding servicefile option and its > regression tests) > in light of the above and committed 0001 patch (regression test of > existing features) > toward next release :) Although it probably won't be ready in time for this release, I've created new 0001 patch (former 0002) which is reflected your review comments. I checked That the patch passes CI of my GitHub repository. Best of luck :) --- Great regards, Ryo Kanbayashi v5-0001-add-servicefile-connection-option-feature.patch Description: Binary data
Re: [PATCH] PGSERVICEFILE as part of a normal connection string
On Tue, Apr 1, 2025 at 6:26 AM Andrew Jackson wrote: > > Hi, > > I am working on a feature adjacent to the connection service functionality > and noticed some issues with the tests introduced in this thread. Basically > they incorrectly invoke the append perl function by passing multiple strings > to append when the function only takes one string to append. This caused the > generated service files to not actually contain any connection parameters. > The tests were only passing because the connect_ok perl function set the > connection parameters as environment variables which covered up the misformed > connection service file. > The attached patch is much more strict in that it creates a dummy database > that is not started and passes all queries though that and tests that the > connection service file correctly overrides the environment variables set by > the dummy databases' query functions Andrew, CC: Michael, Torsten Thank you to find issues the tests. I confirmed points you noticed and validity of your proposed modifications with local execution and internal impl of connect_ok func. - Current usage of append_to_file func is wrong and not appropriate service file is generated - connect_ok perl func set the connection parameters as environment variables which covered up the misformed connection service file - https://github.com/postgres/postgres/blob/ea3f9b6da34a1a4dc2c0c118789587c2a85c78d7/src/test/perl/PostgreSQL/Test/Cluster.pm#L2576 - https://github.com/postgres/postgres/blob/ea3f9b6da34a1a4dc2c0c118789587c2a85c78d7/src/test/perl/PostgreSQL/Test/Cluster.pm#L2120 - https://github.com/postgres/postgres/blob/ea3f9b6da34a1a4dc2c0c118789587c2a85c78d7/src/test/perl/PostgreSQL/Test/Cluster.pm#L1718 - Your dummy node object introduced code works without problem and the code is more strict than current code I'll reflect your notice and suggestion to the patch current I'm working on :) --- Great Regards, Ryo Kanbayashi
Re: [PATCH] PGSERVICEFILE as part of a normal connection string
On Mon, Apr 7, 2025 at 1:10 PM Michael Paquier wrote: > > On Thu, Apr 03, 2025 at 12:36:59AM +0900, Ryo Kanbayashi wrote: > > I'll reflect your notice and suggestion to the patch current I'm > > working on :) > > Thanks for that. > > And I have forgotten to add you as a reviewer of what has been > committed as 2c7bd2ba507e. Sorry for that :/ No problem :) I rebased our patch according to 2c7bd2ba507e. https://commitfest.postgresql.org/patch/5387/ --- Great regards, Ryo Kanbayashi v7-0001-add-servicefile-connection-option-feature.patch Description: Binary data
