Re: Add Pipelining support in psql

On Wed, Apr 16, 2025 at 09:18:01AM -0700, Michael Paquier wrote:
> On Wed, Apr 16, 2025 at 09:31:59PM +0700, a.kozhemyakin wrote:
>> After commit 2cce0fe on master
>>
>> ERROR:  unexpected message type 0x50 during COPY from stdin
>> CONTEXT:  COPY psql_pipeline, line 1
>> Pipeline aborted, command did not run
>> psql: common.c:1510: discardAbortedPipelineResults: Assertion `res == ((void
>> *)0) || result_status == PGRES_PIPELINE_ABORTED' failed.
>> Aborted (core dumped)
>
> Reproduced here, thanks for the report. I'll look at that at the
> beginning of next week, adding an open item for now.

The failure is not related to 2cce0fe. The following sequence fails
as well, as long as we have one SELECT after the COPY to mess up with
the \getresults that causes a PGRES_FATAL_ERROR combined with a
"terminating connection because protocol synchronization was lost" on
the backend side, because the server expects some data while the
client does not send it but psql is not able to cope with this state:
\startpipeline
COPY psql_pipeline FROM STDIN \bind \sendpipeline
SELECT $1 \bind 'val1' \sendpipeline
\syncpipeline
\getresults
\endpipeline

It's actually nice that we are able to emulate such query patterns
with psql using all these meta-commands, I don't think we have
any coverage for the backend synchronization loss case yet like this
one?

2cce0fe makes that easier to reach by allowing more command patterns,
but it's the mix of COPY followed by a SELECT that causes psql to be
confused. All the tests that we have don't check this kind of
scenarios, for COPY TO/FROM, with always use a flush or a sync
followed quickly by \getresults, but we don't have tests where we mix
things.

Anyway, I don't think that there is much we can do under a
PGRES_FATAL_ERROR in this code path when discarding the pipe results.
As far as I can tell, the server has failed the query suddenly and the
whole pipeline flow is borked. The best thing that I can think of is
to discard all the results while decrementing the counters, then let
psql complain about that like in the attached. I've added two tests
in TAP, as these trigger a FATAL in the backend so we cannot use the
normal SQL route, so as we have some coverage.

@Anthonin: Any thoughts or comments, perhaps? A second opinion would
be welcome here.
--
Michael