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

Hi,

On 2025/09/22 11:56, Fujii Masao wrote:
> On Sat, Sep 20, 2025 at 12:21 AM Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>>> While testing, I found that running pgbench with --continue-on-error and
>>> pipeline mode triggers the following assertion failure. Could this be
>>> a bug in the patch?
>>>
>>> ---------------------------------------------------
>>> $ cat pipeline.pgbench
>>> \startpipeline
>>> DO $$
>>> BEGIN
>>> PERFORM pg_sleep(3);
>>> PERFORM pg_terminate_backend(pg_backend_pid());
>>> END $$;
>>> \endpipeline
>>>
>>> $ pgbench -n --debug --verbose-errors -f pipeline.pgbench -c 2 -t 4 -M
>>> extended --continue-on-error
>>> ...
>>> Assertion failed:
>>> (sql_script[st->use_file].commands[st->command]->type == 1), function
>>> commandError, file pgbench.c, line 3081.
>>> Abort trap: 6
>>> ---------------------------------------------------
>>>
>>> When I ran the same command without --continue-on-error,
>>> the assertion failure did not occur.
>>
>> I think this bug was introduced by commit 4a39f87acd6e, which enabled pgbench
>> to retry and added the --verbose-errors option, rather than by this patch itself.
>>
>> The assertion failure occurs in commandError(), which is called to report an error when
>> it can be retried (i.e., serializable failure or deadlock), or when --continue-on-error
>> is used after this patch.
>>
>> Assert(sql_script[st->use_file].commands[st->command]->type == SQL_COMMAND);
>>
>> This assumes the error is always detected during SQL command execution, but
>> that’s not correct, since in pipeline mode, the error can be detected when
>> a \endpipeline meta-command is executed.
>>
>> $ cat deadlock.sql
>> \startpipeline
>> begin;
>> lock b;
>> lock a;
>> end;
>> \endpipeline
>>
>> $ cat deadlock2.sql
>> \startpipeline
>> begin;
>> lock a;
>> lock b;
>> end;
>> \endpipeline
>>
>> $ pgbench --verbose-errors -f deadlock.sql -f deadlock2.sql -c 2 -T 3 -M extended
>> pgbench (19devel)
>> starting vacuum...end.
>> pgbench: pgbench.c:3062: commandError: Assertion `sql_script[st->use_file].commands[st->command]->type == 1' failed.
>>
>> Although one option would be to remove this assertion, if we prefer to keep it,
>> the attached patch fixes the issue.
>
> Thanks for the analysis and the patch!
>
> I think we should fix the issue rather than just removing the assertion.
> I'd like to apply your patch with the following source comment:
>
> ---------------------------
> Errors should only be detected during an SQL command or the \endpipeline
> meta command. Any other case triggers an assertion failure.
> --------------------------
>
>
> With your patch and the continue-on-error patches, running the same pgbench
> command I used to reproduce the assertion failure upthread causes pgbench
> to hang. From my analysis, it enters an infinite loop in discardUntilSync().
> That loop waits for PGRES_PIPELINE_SYNC, but since the connection has already
> been closed, it never arrives, leaving pgbench stuck.
>
> Could this also happen without the continue-on-error patch, or is it a new bug
> introduced by it? Either way, it seems pgbench needs to exit the loop when
> the result status is PGRES_FATAL_ERROR.
>

Thank you for the analysis and the patches.

I think the issue is a new bug because we have transitioned to CSTATE_ABORT
immediately after queries failed, without executing discardUntilSync().

I've attached a patch that fixes the assertion error. The content of v1 patch by
Mr. Nagata is also included. I would appreciate it if you review my patch.

Regards,
Rintaro Ikeda