Horton
Menu

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

Rintaro Ikeda <ikedarintarof@oss.nttdata.com>

View thread
From: Rintaro Ikeda <ikedarintarof@oss.nttdata.com>
To: Yugo Nagata <nagata@sraoss.co.jp>, Fujii Masao <masao.fujii@gmail.com>
Cc: Jakub Wartak <jakub.wartak@enterprisedb.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "slpmcf@gmail.com" <slpmcf@gmail.com>, "boekewurm+postgres@gmail.com" <boekewurm+postgres@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, Srinath Reddy Sadipiralla <srinath2133@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>
Date: 2025-09-20T12:58:04Z
Lists: pgsql-hackers 
Thank you for reviewing the patches.

On 2025/09/19 20:56, Yugo Nagata wrote:
>>>> A client's run is aborted in case of a serious error; for example, the
>>>> connection with the database server was lost or the end of script was reached
>>>> without completing the last transaction.  The client also aborts
>>>> if a meta-command fails, or if an SQL command fails for reasons other than
>>>> serialization or deadlock errors when --continue-on-error is not specified.
>>>> With --continue-on-error, the client does not abort on such SQL errors
>>>> and instead proceeds to the next transaction.  These cases are reported
>>>> as "other failures" in the output.  If the error occurs in a meta-command,
>>>> however, the client still aborts even when this option is specified.
>>>> ----------------------------
>>>
>>> I'm fine with that. This version is clearer.

I also agree with this.

>>
>> Also I'd like to share the review comments for 0002 and 0003.
>>
>> Regarding 0002:
>>
>> - TSTATUS_OTHER_ERROR,
>> + TSTATUS_UNKNOWN_ERROR,
>>
>> You did this rename to avoid confusion with other_sql_errors.
>> I see the intention, but I'm not sure if this concern is really valid
>> and if the rename adds much value. Also, TSTATUS_UNKNOWN_ERROR
>> might be mistakenly assumed to be related to PQTRANS_UNKNOWN,
>> even though they aren't related...
> 
> I don’t have a strong opinion on this, but I think TSTATUS_* is slightly
> related to PQTRANS_*, since getTransactionStatus() determines the TSTATUS
> value based on PQTRANS. There is no one-to-one relationship, of course,
> but it is more related than ESTATUS_OTHER_SQL_ERROR, which is entirely
> separate.
> 
>> But if we agree with this change, I think it should be folded into 0001,
>> since there's no strong reason to keep it separate.
> 
> +1
> 
> I personally don't care if ommiting this change, but I would like to wait 
> for Ikeda-san's response because he is the author of these two patches.
> 

The points you both raise make sense to me.
Changing the macro name is not important for the purpose of the patch, so I now
feel it would be reasonable to drop patch 0002.


Regards,
Rintaro Ikeda

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pgbench: Fix assertion failure with multiple \syncpipeline in pipeline mode.

  2. pgbench: Add --continue-on-error option.

  3. Fix "inconsistent DLL linkage" warning on Windows MSVC

  4. pgbench: Fix error reporting in readCommandResponse().

  5. pgbench: Fix assertion failure with retriable errors in pipeline mode.

  6. Allow pgbench to retry in some cases.