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

On Wednesday, February 20, 2013 5:25 PM Etsuro Fujita wrote:
> Hi Amit,
>
> Thank you for the review.

Etsuro-san, you are welcome.

> > 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.

I have asked this thinking that ReleaseLruFile() may not be useful for
OpenPipeStream,
As I was not sure how the new file descriptors get allocated for popen.
But now again reading popen specs, I got the point that it can be useful.

> > 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.

To be honest, I am not sure whether introducing pre, post processor
terminology is right or not,
But again I shall let committer decide about this point.

> > 6. Can we have one example of this new syntax, it can make it more
> > meaningful.
>
> Done.
>
> Sorry for the long delay.

All the reported issues are handled in the new patch.

I have one small another doubt that in function parse_slash_copy, you
avoided expand tilde
for program case, which I am not sure is the right thing or not.

I am marking this patch as Ready For Committer.

Notes For Committer
-----------------------
1. "Broken pipe" is not handled in case of psql "\copy" command;
This is currently documented
2. Documentation needs to be checked, especially with focus whether
introducing pre, post processor terminology is
Okay.
3. In function parse_slash_copy, expand tilde is avaoided, is it okay?

With Regards,
Amit Kapila.