Thread: RE: ecpg command does not warn COPY ... FROM STDIN;
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
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 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
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