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

Hi,

Thank you for the work!

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

> On Wednesday, February 27, 2013 12:41 AM Heikki Linnakangas wrote:

> > There's one oddity in the psql \copy syntax. This is actually present
> > in earlier versions too, but I think we should fix it now that we
> > extend the syntax:
> >
> > -- Writes to standard output. There's no way to write to a file
> > called "stdout".
> > \copy foo to 'stdout'
> >
> > I think we should change the interpretation of the above so that it
> > writes to a file called "stdout". It's possible that there's an
> > application out there that relies on that to write to stdout, but it's
> > not hard to fix the application. A small note in the release notes
> > might be in order.
> >
> > Also, I think we should require the command to be quoted in \copy. Ie.
> > let's forbid this:
> >
> > \copy pgbench_accounts to program /bin/cat>/dev/null
> >
> > and require that it's written as:
> >
> > \copy pgbench_accounts to program '/bin/cat>/dev/null'
> >
> > We don't require quoting for filenames, which I find a bit weird too,
> > but it seems particularly confusing for a shell command.

Agreed. ISTM that the comment on parse_slash_copy() needs to be changed:

* table name can be double-quoted and can have a schema part.
* column names can be double-quoted.
* filename can be single-quoted like SQL literals.
* command can be single-quoted like SQL literals.

The last line must be:

* command must be single-quoted like SQL literals.

> > Attached is a new version of this patch that I almost committed, but
> > one thing caught my eye at the last minute: The error reporting is not
> > very user-friendly. If the program fails after reading/writing some
> > rows, the reason is printed to the log, but not the user:
> >
> > postgres=# copy foo to program '/tmp/midway-crash';
> > ERROR: could not execute command "/tmp/midway-crash"
> >
> > the log has more details:
> >
> > LOG: child process exited with exit code 10
> > STATEMENT: copy foo to program '/tmp/midway-crash';
> > ERROR: could not execute command "/tmp/midway-crash"
> > STATEMENT: copy foo to program '/tmp/midway-crash';
> >
> > I think we should arrange for the "child process exited with exit code
> > 10" to be printed as errdetail to the client. Similarly, with psql
> > \copy command, the "child process exited with exit code 10" command
> > shouldn't be printed straight to stderr, but should go through
> > psql_error.
> >
> > I'll try to refactor pclose_check tomorrow to do that. Meanwhile,
> > please take a look at the attached if you have the time. I rewrote much
> > of the docs changes, and some comments.

Looks fine to me.

> If there is semicolon at end, it takes it into account for creating
> filename.
> \copy foo to c:\data\foo.out;
> This problem was fixed in previous patch, but I think handling of quotes has
> changed this behavior.

ISTM that the following part at parse_slash_copy() needs to be changed:

/* { 'filename' | PROGRAM 'command' | STDIN | STDOUT | PSTDIN | PSTDOUT } */
token = strtokx(NULL, whitespace, NULL, "'",
0, false, false, pset.encoding);
if (!token)
goto error;

strtokx() in the above should be called in the following way:

token = strtokx(NULL, whitespace, ";", "'",
0, false, false, pset.encoding);

Thanks,

Best regards,
Etsuro Fujita