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
thinkv4 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
someoneremoves “res==NULL” from the “else if”, it will lead to double memory free, because after “break”, PQclear(res)
willbe called again. 

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