Fabien COELHO wrote:
> > 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:
The change mentioned above is not supposed to have any user-visible
effect, compared to the previous version. The success of the command
as a whole, that is reflected by :ERROR, is the result of AND'ing the
successes of different steps of the command. Having handleCopyOut()
succeed does not change the falseness of the aggregated result,
which is still false when it fails to open the output stream.
> But previously we had:
>
> sql> \echo :ROW_COUNT # 0
Previously "COPY... \g file" behaved as "COPY... \g", the argument being
ignored. In this case, and the patch doesn't change that, the code does:
/*
* Suppress status printing if the report would go to the same
* place as the COPY data just went. Note this doesn't
* prevent error reporting, since handleCopyOut did that.
*/
if (copystream == pset.queryFout)
{
PQclear(copy_result);
copy_result = NULL;
}
One effect of clearing that result is that :ROW_COUNT is going to
be set to zero by SetResultVariables(), which explains the above
output.
:ROW_COUNT is incorrect after COPY TO STDOUT but this is a PG11-only
bug, :ROW_COUNT being a recent addition, so maybe we should deal with
this separately, since the current patch is supposed to address all versions?
> 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.
Right, but wether COPY fails because psql can't write the output,
possibly half-way because of a disk full condition, or because the
query was cancelled or the server went down, are these distinctions
meaningful for a script?
If we say yes, how can the user know that the data fetched is
empty or incomplete? We don't have a CLIENT_SIDE_ERROR variable.
Also, the fact that psql retrieves the results when it doesn't have
a valid destination for them is an implementation detail.
I think we could also cancel the query in a way that would be
technically an SQL error, as Ctrl+C would do.
Hiding these details under a generic ERROR=true seems reasonable,
especially since we expose SQLSTATE for fine-grained error checking,
should that be needed.
ERROR=true and SQLSTATE empty is already mentioned as plausible
in SetResultVariables():
SetVariable(pset.vars, "ERROR", "true");
/*
* If there is no SQLSTATE code, use an empty string. This can
happen
* for libpq-detected errors (e.g., lost connection, ENOMEM).
*/
if (code == NULL)
code = "";
SetVariable(pset.vars, "SQLSTATE", code);
> 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 ..." */
The bug we're fixing here is due to missing the point the comment is
making, so being thick seems fair.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
