Re: Unhappy with error handling in psql's handleCopyOut()

On Tue, Feb 11, 2014 at 03:43:08PM -0500, Tom Lane wrote:
> While looking at the pending patch to make psql report a line count
> after COPY, I came across this business in handleCopyOut():
> 
>      * Check command status and return to normal libpq state.  After a
>      * client-side error, the server will remain ready to deliver data.  The
>      * cleanest thing is to fully drain and discard that data.  If the
>      * client-side error happened early in a large file, this takes a long
>      * time.  Instead, take advantage of the fact that PQexec() will silently
>      * end any ongoing PGRES_COPY_OUT state.  This does cause us to lose the
>      * results of any commands following the COPY in a single command string.
>      * It also only works for protocol version 3.  XXX should we clean up
>      * using the slow way when the connection is using protocol version 2?
> 
> which git blames on commit 08146775 (committed by Alvaro on behalf of
> Noah).
> 
> This does not make me happy.  In the first place, we have not dropped
> support for protocol version 2.

As you may realize, that commit helped protocol version 3 most but also
strictly improved things for protocol version 2.  I didn't feel the need to
improve them both to the same extent, a judgment that still seems reasonable.

> In the second place, I fail to see
> what the advantage is of kluging things like this.  The main costs of
> draining the remaining COPY data are the server-side work of generating
> the data and the network transmission costs, neither of which will go
> away with this technique.

Agreed.

From your commit b8f00a4 of 2014-02-13:
> +     * If for some reason libpq is still reporting PGRES_COPY_OUT state, we
> +     * would like to forcibly exit that state, since our caller would be
> +     * unable to distinguish that situation from reaching the next COPY in a
> +     * command string that happened to contain two consecutive COPY TO STDOUT
> +     * commands.  However, libpq provides no API for doing that, and in
> +     * principle it's a libpq bug anyway if PQgetCopyData() returns -1 or -2
> +     * but hasn't exited COPY_OUT state internally.  So we ignore the
> +     * possibility here.
>       */

PQgetCopyData() returns -2 without exiting COPY_OUT when its malloc() call
fails.  My submission of the patch that became commit 08146775 included a test
procedure for getting an infinite loop in handleCopyOut(); that commit fixed
the infinite loop.  After commit b8f00a4, the same test procedure once again
initiates an infinite loop.  To a large extent, that's bad luck on the part of
commit b8f00a4.  I bet malloc() failure in pqCheckInBufferSpace() could
initiate an infinite loop even with the longstanding 9.2/9.3 code.

> So I'm thinking we should revert this kluge
> and just drain the data straightforwardly, which would also eliminate
> the mentioned edge-case misbehavior when there were more commands in
> the query string.
> 
> Is there a reason I'm overlooking not to do this?

The moral of the infinite loop test case story is that draining the data
straightforwardly has strictly more ways to fail.  Consequently, commit
b8f00a4 improved some scenarios while regressing others.  Overall, I do think
it was an improvement.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com