public inbox for [email protected]
help / color / mirror / Atom feedFrom: Anthonin Bonnefoy <[email protected]>
To: Michael Paquier <[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: Tue, 22 Apr 2025 14:37:19 +0200
Message-ID: <CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_+3g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAO6_Xqroaof=Gu_NCg4bZfpdgwOjNW9fg753cuuts-FBnv9UGw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
On Tue, Apr 22, 2025 at 2:06 AM Michael Paquier <[email protected]> wrote:
> I am wondering if we could not be smarter with the handling of
> the counters, but I really doubt that there is much more we can do
> under a PGRES_FATAL_ERROR.
One thing that bothers me is that the reported error is silently
discarded within discardAbortedPipelineResults.
psql -f bug_assertion.sql
psql:bug_assertion.sql:7: ERROR: unexpected message type 0x50 during
COPY from stdin
CONTEXT: COPY psql_pipeline, line 1
psql:bug_assertion.sql:7: Pipeline aborted, command did not run
This should ideally report the "FATAL: terminating connection because
protocol synchronization was lost" sent by the backend process.
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.
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.
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.
[1]: https://cirrus-ci.com/task/5051031505076224?logs=check_world#L240-L246
Attachments:
[application/octet-stream] v02-0001-PATCH-psql-Fix-assertion-failure-with-pipeline-m.patch (3.7K, ../CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_+3g@mail.gmail.com/2-v02-0001-PATCH-psql-Fix-assertion-failure-with-pipeline-m.patch)
download | inline diff:
From 5d37f2616c82c6525d656149d383ef01a6d7518c Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Mon, 21 Apr 2025 15:17:43 +0900
Subject: [PATCH] psql: Fix assertion failure with pipeline mode
---
src/bin/psql/common.c | 17 ++++++++++++
src/bin/psql/t/001_basic.pl | 55 ++++++++++++++++++++++++++++++++++---
2 files changed, 68 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 21d660a8961..0aab02ee32e 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 739cb439708..8d258c00c5e 100644
--- a/src/bin/psql/t/001_basic.pl
+++ b/src/bin/psql/t/001_basic.pl
@@ -34,11 +34,13 @@ sub psql_fails_like
{
local $Test::Builder::Level = $Test::Builder::Level + 1;
- my ($node, $sql, $expected_stderr, $test_name) = @_;
+ my ($node, $sql, $expected_stderr, $test_name, $replication) = @_;
+
+ # Use the context of a WAL sender, if requested by the caller.
+ $replication = '' unless defined($replication);
- # Use the context of a WAL sender, some of the tests rely on that.
my ($ret, $stdout, $stderr) =
- $node->psql('postgres', $sql, replication => 'database');
+ $node->psql('postgres', $sql, replication => $replication);
isnt($ret, 0, "$test_name: exit code not 0");
like($stderr, $expected_stderr, "$test_name: matches");
@@ -79,7 +81,8 @@ psql_fails_like(
$node,
'START_REPLICATION 0/0',
qr/unexpected PQresultStatus: 8$/,
- 'handling of unexpected PQresultStatus');
+ 'handling of unexpected PQresultStatus',
+ 'database');
# test \timing
psql_like(
@@ -481,4 +484,48 @@ 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()');
+psql_fails_like(
+ $node,
+ qq{\\startpipeline
+COPY psql_pipeline FROM STDIN;
+SELECT 'val1';
+\\syncpipeline
+\\getresults
+\\endpipeline},
+ qr/server closed the connection unexpectedly/,
+ 'handling of protocol synchronization loss with pipelines');
+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/,
+ 'handling of protocol synchronization loss with pipelines');
+
+# This time, test without the \getresults
+psql_fails_like(
+ $node,
+ qq{\\startpipeline
+COPY psql_pipeline FROM STDIN;
+SELECT 'val1';
+\\syncpipeline
+\\endpipeline},
+ qr/server closed the connection unexpectedly/,
+ 'handling of protocol synchronization loss with pipelines');
+psql_fails_like(
+ $node,
+ qq{\\startpipeline
+COPY psql_pipeline FROM STDIN \\bind \\sendpipeline
+SELECT 'val1' \\bind \\sendpipeline
+\\syncpipeline
+\\endpipeline},
+ qr/server closed the connection unexpectedly/,
+ 'handling of protocol synchronization loss with pipelines');
+
done_testing();
--
2.39.5 (Apple Git-154)
view thread (30+ 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: <CAO6_XqoQaZVHEw1dFgvOxwectqsNo4QE1tQ6rLD-YUzFpT_+3g@mail.gmail.com>
* 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