On Thu, 13 Nov 2025 11:55:25 +0900 Fujii Masao <[email protected]> wrote:
> On Wed, Nov 12, 2025 at 6:34 PM Yugo Nagata <[email protected]> wrote: > > I've attached an updated patch. > > Thanks for updating the patch! > > > The comment for the PQpipelineSync() call has been also updated to clarify > > why it is necessary. > > + /* > + * If a PGRES_PIPELINE_SYNC is followed by something other than > + * PGRES_PIPELINE_SYNC or NULL, another PGRES_PIPELINE_SYNC will > + * eventually follow. > + */ > > LGTM. I'd like to append "Reset received_sync to false to wait for > it." into this comment. > > > > In addition, I added a connection status check in the loop to avoid an > > infinte loop that waiting for PQpipelineSync after a connection failure. > > Would it be better to move this status check right after PQgetResult() > so that connection failures can be detected regardless of what result > it returns? > > + pg_log_error("client %d aborted: the backend died while rolling back > the failed transaction after", > > The trailing “after” seems unnecessary. > > Since there's no guarantee the backend actually died in this case, > it might be better to use something like "client %d aborted while rolling back > the transaction after an error; perhaps the backend died while processing" > which matches the wording used under CSTATE_WAIT_ROLLBACK_RESULT > in advanceConnectionState(). Thank you for your review! I've attached an updated patch reflecting your suggestion. Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
>From c7b3d71880dc1bbd9efbf22f663c30a0b7e01a9a Mon Sep 17 00:00:00 2001 From: Yugo Nagata <[email protected]> Date: Tue, 11 Nov 2025 10:14:30 +0900 Subject: [PATCH v3] pgbench: Fix assertion failure with multiple \syncpipeline in pipeline mode. When running pgbench with a custom script that triggered retriable errors (e.g., deadlock errors) followed by multiple \syncpipeline commands in pipeline mode, an assertion failure could occur: pgbench.c:3594: discardUntilSync: Assertion `res == ((void *)0)' failed This happened because discardUntilSync() did not expect that a result other than NULL (e.g. PGRES_TUPLES_OK) might be received after \syncpipeline. This commit fixes the assertion failure by resetting the receive_sync flag and continueing to discard results to ensure that all results are discarded until the last sync point. Also, if the connection was unexpectedly closed, this function could get stuck in an infinite loop waiting for PGRES_PIPELINE_SYNC, which would never be received. To fix this, exit the loop immediately if a connection failure is detected. --- src/bin/pgbench/pgbench.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index d8764ba6fe0..7d50ee38399 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -3563,14 +3563,18 @@ doRetry(CState *st, pg_time_usec_t *now) } /* - * Read results and discard it until a sync point. + * Read and discard results until the last sync point. */ static int discardUntilSync(CState *st) { bool received_sync = false; - /* send a sync */ + /* + * Send a sync to ensure at least one PGRES_PIPELINE_SYNC is received + * and to avoid an infinite loop, since all earlier ones may have + * already been received. + */ if (!PQpipelineSync(st->con)) { pg_log_error("client %d aborted: failed to send a pipeline sync", @@ -3578,21 +3582,23 @@ discardUntilSync(CState *st) return 0; } - /* receive PGRES_PIPELINE_SYNC and null following it */ + /* receive the last PGRES_PIPELINE_SYNC and null following it */ for (;;) { PGresult *res = PQgetResult(st->con); + if (PQstatus(st->con) == CONNECTION_BAD) + { + pg_log_error("client %d aborted while rolling back the transaction after an error; perhaps the backend died while processing", + st->id); + PQclear(res); + return 0; + } + if (PQresultStatus(res) == PGRES_PIPELINE_SYNC) received_sync = true; - else if (received_sync) + else if (received_sync && res == NULL) { - /* - * PGRES_PIPELINE_SYNC must be followed by another - * PGRES_PIPELINE_SYNC or NULL; otherwise, assert failure. - */ - Assert(res == NULL); - /* * Reset ongoing sync count to 0 since all PGRES_PIPELINE_SYNC * results have been discarded. @@ -3601,6 +3607,15 @@ discardUntilSync(CState *st) PQclear(res); break; } + else + { + /* + * If a PGRES_PIPELINE_SYNC is followed by something other than + * PGRES_PIPELINE_SYNC or NULL, another PGRES_PIPELINE_SYNC will + * eventually follow. Reset received_sync to false to wait for it. + */ + received_sync = false; + } PQclear(res); } -- 2.43.0
