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

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