Bonjour Daniel,
>> I took a quick look at this patch.
>
> PFA an updated patch addressing your comments and Fabien's.
About this v2.
Applies cleanly, compiles cleanly, "make check" ok.
No tests, but Tom suggests this does not belong here: the committer is
right:-)
I tested the feature manually, and I noticed that with your patch
ROW_COUNT is set more consistently:
sql> COPY pgbench_branches TO STDOUT \g /dev/null # COPY 10
sql> \echo :ROW_COUNT # 10
But previously we had:
sql> \echo :ROW_COUNT # 0
This is an improvement, although I'm not sure where it comes from.
> I've also changed handleCopyOut() to return success if it
> could pump the data without writing it out locally for lack of
> an output stream. It seems to make more sense like that.
I'm hesitating on this one.
On the one hand the SQL query is executed, on the other hand the \g
command partially failed. There is a debatable side effect: If there is an
error, eg:
sql> COPY pgbench_branches TO STDOUT \g /NIET
/NIET: Permission denied
sql> \echo :ERROR :ROW_COUNT # true 0
I understand from the code that the COPY is really executed, so the ERROR
and so ROW_COUNT about the SQL should reflect that. Basically the change
makes the client believe that there is an SQL error whereas the error is
on the client.
Does anyone else have an opinion?
About the code:
I'm unclear whether the project policy accepts "foo" for "foo != NULL",
whether the later is prefered, or whether it does not care about it.
/*
* COPY TO STDOUT \g [|]file may be used as an alternative
* to \copy
*/
The later part of the comment is already stated in the documentation, I'm
not sure it is worth repeating it here. I'd suggest a simpler /* handle
"COPY TO STDOUT \g ..." */
> While adding the note to the doc I've noticed that the other \copy
> tip says:
>
> "This operation is not as efficient as the SQL COPY command because
> all data must pass through the client/server connection. For large
> amounts of data the SQL command might be preferable.
>
> It doesn't specify that it's for COPY TO/FROM file, not COPY TO
> STDOUT/FROM STDIN. Of course the latter would rank the same as \copy
> with respect to client/server throughput. Should this tip be more
> specific?
Yep.
This tip also overlooks that the client and server are not necessary on
the same host with the same permissions: it seems to say "prefer COPY to
\copy", while the alternative may work only under special conditions which
are not hinted about in any way.
--
Fabien.