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

On Wednesday, January 23, 2013 5:36 PM Etsuro Fujita wrote:
> Hi Amit,

> Thank you for your review.  I’ve rebased and updated the patch.  Please
find attached the patch.

>> 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.
The other changes done by you in revised patch are fine.

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)

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;

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

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

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>

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

With Regards,
Amit Kapila.