Re: Review : Add hooks for pre- and post-processor executables for COPY and \copy

Hi Amit,

Thank you for the review.

> From: Amit Kapila [mailto:amit(dot)kapila(at)huawei(dot)com]

> >> Test case issues:
> >> ------------------
> >> 1. "Broken pipe" is not handled in case of psql "\copy" command;
> >>     Issue are as follows:
> >>         Following are verified on SuSE-Linux 10.2.
> >>         1) psql is exiting when "\COPY xxx TO" command is issued and
> command/script is not found
> >>                 When popen is called in write mode it is creating valid
> file descriptor and when it tries to write to file "Broken pipe" error is >
> coming which is not handled.
> >>                         psql# \copy pgbench_accounts TO PROGRAM
> '../compress.sh pgbench_accounts4.txt'
> >>         2) When "\copy" command is in progress then program/command is
> killed/"crashed due to any problem"
> >>            psql is exiting.
>
> >This is a headache.  I have no idea how to solve this.
>
> I think we can keep it for committer to take a call on this issue.

Agreed.

> I have found few more minor issues as below:
>
> 1. The comment above do_copy can be modified to address the new
> functionality it can handle.
> /*
> * Execute a \copy command (frontend copy). We have to open a file, then
> * submit a COPY query to the backend and either feed it data from the
> * file or route its response into the file.
> */
> bool
> do_copy(const char *args)

Done.

> 2.
> @@ -256,8 +273,14 @@ do_copy(const char *args)
> + if (options->file == NULL && options->program)
> + {
> + psql_error("program is not supported to stdout/pstdout or
> from stdin/pstdin\n");
> + return false;
> + }
>
> should call free_copy_options(options); before return false;

Good catch! Done.

> 3. \copy command doesn't need semicolon at end, however it was working
> previous to your patch, but
> now it is giving error.
> postgres=# \copy t1 from 'e:\pg_git_code\Data\t1_Data.txt';
> e:/pg_git_code/Data/t1_Data.txt';: No such file or directory
> e:/pg_git_code/Data/t1_Data.txt';: No such file or directory

Sorry, I've fixed the bug.

> 4. Please check if OpenPipeStream() it needs to call
> if (ReleaseLruFile()),

OpenPipeStream() calls ReleaseLruFile() by itself if necessary.

> 5. Following in copy.sgml can be changed to make more meaningful as the
> first line looks little adhoc.
> + <para>
> + The command that input comes from or that output goes to.
> + The command for COPY FROM, which input comes from, must write its
> output
> + to standard output. The command for COPY TO, which output goes to,
> must
> + read its input from standard input.
> + </para>

I've struggled to make the document more meaningful.

> 6. Can we have one example of this new syntax, it can make it more
> meaningful.

Done.

Sorry for the long delay.

Best regards,
Etsuro Fujita

> With Regards,
> Amit Kapila.
>
>