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 <stdio.h>
#include <stdlib.h>
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.dev@gmail.com
https://github.com/ryogrid
Attachments:
copy_from_stdin_no_warning.patchapplication/octet-stream; name=copy_from_stdin_no_warning.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 71f7ad26ad..b732f1a355 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,7 +240,7 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: addon var_value NumericOnly
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
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 implementedI 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)
<kuroda.hayato@fujitsu.com> wrote:
Show quoted text
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 implementedI feel we can change like "COPY FROM STDIN/STDOUT...", or prepare two messages.
Thought?Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
copy_from_stdin_no_warning2.patchapplication/octet-stream; name=copy_from_stdin_no_warning2.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 71f7ad26ad..e53929dd84 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,9 +240,9 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
- mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
+ mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN/STDOUT is not implemented");
ECPG: addon var_value NumericOnly
if (@1[0] == '$')
@$ = "$0";
On 2025/01/08 23:04, Ryo Kanbayashi wrote:
But it is not working.
ecpg command fails to notice though code like above exits on pgc code.
This issue seems to have been introduced in commit 3d009e45bd. Before that,
as per my testing, ecpg successfully detected COPY FROM STDIN and reported
a warning. It seems the fix will need to be back-patched to all supported versions.
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.
As for COPY FROM STDOUT, while it's possible, mentioning it might be
confusing since it's not official syntax. So I'm not sure if this is
good idea.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
On 2025/01/08 23:04, Ryo Kanbayashi wrote:
But it is not working.
ecpg command fails to notice though code like above exits on pgc code.
This issue seems to have been introduced in commit 3d009e45bd.
Indeed :-(
As for COPY FROM STDOUT, while it's possible, mentioning it might be
confusing since it's not official syntax. So I'm not sure if this is
good idea.
There's another problem: the correct syntax is COPY TO STDOUT,
and that won't trigger this warning either.
I'm inclined to drop the test on @5 altogether, and just check for
"stdin" or "stdout" in @7. There is no variant of those that will
work. (But maybe we should allow it if opt_program isn't empty?)
The warning message could perhaps be written
"COPY FROM STDIN/TO STDOUT is not implemented".
regards, tom lane
On 2025/01/09 0:42, Tom Lane wrote:
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
On 2025/01/08 23:04, Ryo Kanbayashi wrote:
But it is not working.
ecpg command fails to notice though code like above exits on pgc code.This issue seems to have been introduced in commit 3d009e45bd.
Indeed :-(
As for COPY FROM STDOUT, while it's possible, mentioning it might be
confusing since it's not official syntax. So I'm not sure if this is
good idea.There's another problem: the correct syntax is COPY TO STDOUT,
and that won't trigger this warning either.
ISTM that ecpg supports COPY TO STDOUT and includes the regression test "copystdout" for it. No?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
On 2025/01/09 0:42, Tom Lane wrote:
There's another problem: the correct syntax is COPY TO STDOUT,
and that won't trigger this warning either.
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.
regards, tom lane
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 PROGRAMSo 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]https://www.postgresql.org/docs/devel/sql-copy.html 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
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)
<kuroda.hayato@fujitsu.com> wrote:
Show quoted text
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 PROGRAMSo 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
Attachments:
copy_from_stdin_no_warning.patchapplication/octet-stream; name=copy_from_stdin_no_warning.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 71f7ad26ad..b732f1a355 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,7 +240,7 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: addon var_value NumericOnly
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.
BTW, regarding the discussion on the list, please avoid top-posting;
bottom-posting is the preferred style on this mailing list.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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
Attachments:
copy_from_stdin_no_warning_for_master.patchapplication/octet-stream; name=copy_from_stdin_no_warning_for_master.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 71f7ad26ad..b732f1a355 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,7 +240,7 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: addon var_value NumericOnly
On Thu, Jan 9, 2025 at 9:27 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> 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
Attachments:
copy_from_stdin_no_warning_for_master2.patchapplication/octet-stream; name=copy_from_stdin_no_warning_for_master2.patchDownload
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,7 +240,7 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: addon var_value NumericOnly
On Fri, Jan 10, 2025 at 5:45 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote:
On Thu, Jan 9, 2025 at 9:27 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> 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
Attachments:
copy_from_stdin_no_warning_for_master3.patchapplication/octet-stream; name=copy_from_stdin_no_warning_for_master3.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 71f7ad26ad..b732f1a355 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,7 +240,7 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: addon var_value NumericOnly
On Thu, Jan 9, 2025 at 9:27 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> 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
Attachments:
copy_from_stdin_no_warning_for_master3.patchapplication/octet-stream; name=copy_from_stdin_no_warning_for_master3.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 71f7ad26ad..b732f1a355 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -240,7 +240,7 @@ ECPG: block where_or_current_clause WHERE CURRENT_P OF cursor_name
@$ = cat_str(2, "where current of", cursor_marker);
}
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 &&
+ if (strcmp(@5, "from") == 0 &&
(strcmp(@7, "stdin") == 0 || strcmp(@7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: addon var_value NumericOnly
copy_from_stdin_no_warning_for_stables.patchapplication/octet-stream; name=copy_from_stdin_no_warning_for_stables.patchDownload
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index 300381eaad5..feb61d11a97 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -234,7 +234,7 @@ ECPG: where_or_current_clauseWHERECURRENT_POFcursor_name block
$$ = cat_str(2,mm_strdup("where current of"), cursor_marker);
}
ECPG: CopyStmtCOPYopt_binaryqualified_nameopt_column_listcopy_fromopt_programcopy_file_namecopy_delimiteropt_withcopy_optionswhere_clause addon
- if (strcmp($6, "from") == 0 &&
+ if (strcmp($5, "from") == 0 &&
(strcmp($7, "stdin") == 0 || strcmp($7, "stdout") == 0))
mmerror(PARSE_ERROR, ET_WARNING, "COPY FROM STDIN is not implemented");
ECPG: var_valueNumericOnly addon
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: /messages/by-id/CANOn0Ez_t5uDCUEV8c1YORMisJiU5wu681eEVZzgKwOeiKhkqQ@mail.gmail.com
-------------------
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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Sun, Jan 12, 2025 at 12:56 PM Fujii Masao
<masao.fujii@oss.nttdata.com> 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: /messages/by-id/CANOn0Ez_t5uDCUEV8c1YORMisJiU5wu681eEVZzgKwOeiKhkqQ@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 branchesShould 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
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 branchesShould 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?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Wed, Jan 15, 2025 at 1:34 AM Fujii Masao <masao.fujii@oss.nttdata.com> 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 branchesShould 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
On 2025/01/17 17:46, Ryo Kanbayashi wrote:
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 :)
Thanks!
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.
I've marked this entry as committed.
Please feel free to create a new CF entry for the regression test patch for ecpg.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
🙏
Ryo さんが Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>
でリアクションしました
2025年1月17日(金) 午後10:51 Fujii Masao <masao.fujii@oss.nttdata.com>:
Show quoted text
On 2025/01/17 17:46, Ryo Kanbayashi wrote:
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 :)Thanks!
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.
I've marked this entry as committed.
Please feel free to create a new CF entry for the regression test patch
for ecpg.Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION