Horton
Menu

Re: Add Pipelining support in psql

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Cc: "a.kozhemyakin" <a.kozhemyakin@postgrespro.ru>, Daniel Verite <daniel@manitou-mail.org>, Jelte Fennema-Nio <postgres@jeltef.nl>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2025-04-23T07:13:14Z
Lists: pgsql-hackers

Attachments

On Tue, Apr 22, 2025 at 02:37:19PM +0200, Anthonin Bonnefoy wrote:
> Also, we still have a triggered assertion failure with the following:
> CREATE TABLE psql_pipeline(a text);
> \startpipeline
> COPY psql_pipeline FROM STDIN;
> SELECT 'val1';
> \syncpipeline
> \endpipeline
> ...
> Assertion failed: (pset.piped_syncs == 0), function
> ExecQueryAndProcessResults, file common.c, line 2153.

Right.  I didn't think about the case of a \endpipeline that fetches
all the results by itself.

> A possible alternative could be to abort discardAbortedPipelineResults
> when we encounter a res != NULL + FATAL error and let the outer loop
> handle it. As you said, the pipeline flow is borked so there's not
> much to salvage. The outer loop would read and print all error
> messages until the closed connection is detected. Then,
> CheckConnection will reset the connection which will reset the
> pipeline state.

Sounds like a better idea seen from here, yes.

> While testing this change, I was initially looking for the "FATAL:
> terminating connection because protocol synchronization was lost"
> message in the tests. However, this was failing on Windows[1] as the
> FATAL message wasn't reported on stderr. I'm not sure why yet.

Hmm.  I vaguely recall that there could be some race condition here
with the attempt to catch up the FATAL message once the server tries
to shut down the connection..

Anyway, I agree that it would be nice to track that this specific
error message is generated in the server.  How about checking the
server logs instead, using a slurp_file() with an offset of the log
file before running each pipeline sequence?  We should use a few
wait_for_log() calls, I think, to be extra careful with the timings
where psql_fails_like() gives up, and I'm worried that this could be
unstable on slow machines.  Something like the attached seems stable
enough here.  What do you think?

The tweak for psql_fails_like() was kind of independent of the rest of
the fix, so I have applied that as a commit of its own.  I am not
convinced about the addition of a 4th test where we use the queries
with semicolons without a \getresults between the sync and the end.
--
Michael

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. psql: Fix assertion failures with pipeline mode

  2. psql: Rework TAP routine psql_fails_like() to define WAL sender context

  3. psql: Split extended query protocol meta-commands in --help=commands

  4. psql: Improve descriptions of \\flush[request] in --help

  5. psql: Fix incorrect status code returned by \getresults

  6. psql: Allow queries terminated by semicolons while in pipeline mode

  7. psql: Add \sendpipeline to send query buffers while in a pipeline

  8. psql: Fix memory leak with \gx used within a pipeline

  9. psql: Add pipeline status to prompt and some state variables

  10. Add more tests for utility commands in pipelines

  11. psql: Add support for pipelines

  12. Add braces for if block with large comment in psql's common.c