Re: Alternative to \copy in psql modelled after \g - Mailing list pgsql-hackers

Bonjour Daniel,

> :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?

ISTM that the patch is considered a bug fix, so it shoud be applied to 
pg11 and fix it?

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

It could if the SQL command has side effects, but probably this does not 
apply to COPY TO which cannot have.

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

Yep. Maybe we should.

The documentation states that ERROR is about SQL, not psql internal stuff:

  ERROR true if the last SQL query failed, false if it succeeded.
        See also SQLSTATE.

And this is somehow the behavior on all other SQL commands, with or 
without your patch:

   sql> SELECT 1 \g /BAD
   /BAD: Permission denied

   sql> \echo :ERROR
   false

Basically, with your patch, the behavior becomes inconsistent between COPY 
and other SQL commands, so there is something to fix.

Given that, I see two options:

(1) document ERROR as being muddy, i.e. there has been some error which 
may be SQL or possibly client side. Although SQLSTATE would still allow to 
know whether an SQL error occured, there is still no client side 
expressions, and even if I had moved pgbench expressions to psql they 
would still need to be extended to handle strings. This suggest that maybe 
there could be an SQL_ERROR boolean which does store whether SQL succeeded 
or not, and possibly a CLIENT_ERROR on the side, and ERROR = SQL_ERROR OR 
CLIENT_ERROR.

(2) keep ERROR as is, i.e. about SQL, and add some CLIENT_ERROR, but then 
I see another issue, if it is *only* the client error, then it should be 
true if there is an SQL error, thus checking if there is any error becomes 
ERROR OR CLIENT_ERROR, which is muddy as well especially as there are no 
client-side expressions in psql.

> Also, the fact that psql retrieves the results when it doesn't have
> a valid destination for them is an implementation detail.

Yep, but if there are side effects, a script could want to know if they 
occured?

> I think we could also cancel the query in a way that would be
> technically an SQL error, as Ctrl+C would do.

Hmmm.

> Hiding these details under a generic ERROR=true seems reasonable,
> especially since we expose SQLSTATE for fine-grained error checking,
> should that be needed.

Yep, but no expression.

> ERROR=true and SQLSTATE empty is already mentioned as plausible
> in SetResultVariables():

Indeed. This suggest that ERROR is already muddy, contrary to the 
documentation.

>     SetVariable(pset.vars, "ERROR", "true");
>     if (code == NULL)
>         code = "";
>     SetVariable(pset.vars, "SQLSTATE", code);

Overall, ISTM that it should point to solution (1).

  - document that ERROR is muddy, "some SQL or client error occured"
  - add SQL_ERROR, which is always about SQL error and nothing else
  - add CLIENT_ERROR, same
  - make the behavior consistent for all SQL command, COPY & others

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

I would not be, because ISTM that mentionning that COPY TO STDOUT is 
specially handled already points clearly to the previous issue. No big 
deal.

-- 
Fabien.