public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Anthonin Bonnefoy <[email protected]>
Cc: a.kozhemyakin <[email protected]>
Cc: Daniel Verite <[email protected]>
Cc: Jelte Fennema-Nio <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Add Pipelining support in psql
Date: Wed, 23 Apr 2025 16:13:14 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_+3g@mail.gmail.com>
References: <CAO6_Xqroaof=Gu_NCg4bZfpdgwOjNW9fg753cuuts-FBnv9UGw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_+3g@mail.gmail.com>
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
Attachments:
[text/x-diff] v3-0001-psql-Fix-assertion-failure-with-pipeline-mode.patch (3.0K, ../[email protected]/2-v3-0001-psql-Fix-assertion-failure-with-pipeline-mode.patch)
download | inline diff:
From 2641356ffacc0c69779a6252a8072f41521c0f3f Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Wed, 23 Apr 2025 15:52:24 +0900
Subject: [PATCH v3] psql: Fix assertion failure with pipeline mode
---
src/bin/psql/common.c | 17 +++++++++++++
src/bin/psql/t/001_basic.pl | 49 +++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 21d660a8961a..0aab02ee32e6 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1478,6 +1478,23 @@ discardAbortedPipelineResults(void)
*/
return res;
}
+ else if (res != NULL && result_status == PGRES_FATAL_ERROR)
+ {
+ /*
+ * We have a fatal error sent by the backend and we can't recover
+ * from this state. Instead, return the last fatal error and let
+ * the outer loop handle it.
+ */
+ PGresult *fatal_res PG_USED_FOR_ASSERTS_ONLY;
+
+ /*
+ * Fetch result to consume the end of the current query being
+ * processed.
+ */
+ fatal_res = PQgetResult(pset.db);
+ Assert(fatal_res == NULL);
+ return res;
+ }
else if (res == NULL)
{
/* A query was processed, decrement the counters */
diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl
index b0e4919d4d71..3cada3ba959b 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -483,4 +483,53 @@ psql_like($node, "copy (values ('foo'),('bar')) to stdout \\g | $pipe_cmd",
my $c4 = slurp_file($g_file);
like($c4, qr/foo.*bar/s);
+# Tests with pipelines. These trigger FATAL failures in the backend,
+# so they cannot be tested through the SQL regression tests.
+$node->safe_psql('postgres', 'CREATE TABLE psql_pipeline()');
+my $log_location = -s $node->logfile;
+psql_fails_like(
+ $node,
+ qq{\\startpipeline
+COPY psql_pipeline FROM STDIN;
+SELECT 'val1';
+\\syncpipeline
+\\getresults
+\\endpipeline},
+ qr/server closed the connection unexpectedly/,
+ 'protocol sync loss in pipeline: direct COPY, SELECT, sync and getresult'
+);
+$node->wait_for_log(
+ qr/FATAL: .*terminating connection because protocol synchronization was lost/,
+ $log_location);
+
+$log_location = -s $node->logfile;
+psql_fails_like(
+ $node,
+ qq{\\startpipeline
+COPY psql_pipeline FROM STDIN \\bind \\sendpipeline
+SELECT 'val1' \\bind \\sendpipeline
+\\syncpipeline
+\\getresults
+\\endpipeline},
+ qr/server closed the connection unexpectedly/,
+ 'protocol sync loss in pipeline: bind COPY, SELECT, sync and getresult');
+$node->wait_for_log(
+ qr/FATAL: .*terminating connection because protocol synchronization was lost/,
+ $log_location);
+
+# This time, test without the \getresults.
+$log_location = -s $node->logfile;
+psql_fails_like(
+ $node,
+ qq{\\startpipeline
+COPY psql_pipeline FROM STDIN;
+SELECT 'val1';
+\\syncpipeline
+\\endpipeline},
+ qr/server closed the connection unexpectedly/,
+ 'protocol sync loss in pipeline: COPY, SELECT and sync');
+$node->wait_for_log(
+ qr/FATAL: .*terminating connection because protocol synchronization was lost/,
+ $log_location);
+
done_testing();
--
2.49.0
[application/pgp-signature] signature.asc (833B, ../[email protected]/3-signature.asc)
download
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add Pipelining support in psql
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox