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

Hi Amit,

Thank you for your careful review!

> -----Original Message-----
> From: Amit Kapila [mailto:amit(dot)kapila(at)huawei(dot)com]
> Sent: Friday, February 22, 2013 7:18 PM
> To: 'Etsuro Fujita'; 'pgsql-hackers'
> Subject: RE: [HACKERS] 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.

Agreed.

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

Sorry, I'm not sure that, too. I'd like to leave this for committers.

> I am marking this patch as Ready For Committer.

Thanks!

Best regards,
Etsuro Fujita

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