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
From 2641356ffacc0c69779a6252a8072f41521c0f3f Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
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
signature.asc
Description: PGP signature
