Re: Suggestion to add --continue-client-on-abort option to pgbench

> On Nov 13, 2025, at 21:55, Fujii Masao <masao.fujii@gmail.com> wrote:
> 
> On Thu, Nov 13, 2025 at 4:09 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:
>> Thank you for your review!
>> I've attached an updated patch reflecting your suggestion.
> 
> Thanks for updating the patch! LGTM.
> 
> You mentioned that the assertion failure could occur when using \syncpipeline,
> but it seems that multiple PGRES_PIPELINE_SYNC results can also appear
> even without it, which can still trigger the same issue. For example,
> I was able to reproduce the assertion failure in v16 (which doesn't support
> \syncpipeline) with the following setup:
> 
> --------------------------------
> $ cat deadlock.sql
> \startpipeline
> select * from a order by i for update;
> select 1;
> \endpipeline
> 
> $ cat deadlock2.sql
> \startpipeline
> select * from a order by i desc for update;
> select 1;
> \endpipeline
> 
> $ psql -c "create table a (i int primary key); insert into a
> values(generate_series(1,1000));"
> 
> $ pgbench -n -j 4 -c 4 -T 5 -M extended -f deadlock.sql -f deadlock2.sql
> ...
> Assertion failed: (res == ((void *)0)), function discardUntilSync,
> file pgbench.c, line 3479.
> --------------------------------
> 
> So I've updated the commit message to clarify that while using \syncpipeline
> makes the failure more likely, it can still occur without it. Since the issue
> can also happen in v15 and v16 (which both lack \syncpipeline), I plan to
> backpatch the fix to v15. The failure doesn't occur in v14 because it doesn't
> support retriable error retries.
> 
> I've also made a few cosmetic tweaks to the patch. Attached is the updated
> version, which I plan to push.
> 
> Regards,
> 
> -- 
> Fujii Masao
> <v4-0001-pgbench-PG15-PG16-Fix-assertion-failure-when-discarding-res.txt><v4-0001-pgbench-Fix-assertion-failure-when-discarding-res.patch>

I think I was misunderstanding that “\syncpipeline” would recover the transaction. Once the confusion is resolved, I think v4 patch is overall good. Only one small comment:

```
+		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 +3610,15 @@ discardUntilSync(CState *st)
 			PQclear(res);
 			break;
 		}
```

As we now add “res==NULL” to the “else if”, once entering "else if (received_sync && res == NULL)”, res must be NULL, so "PQclear(res);” should be deleted. Leaving it there doesn’t harm today, but is error-prone, because if in future someone removes “res==NULL” from the “else if”, it will lead to double memory free, because after “break”, PQclear(res) will be called again.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/