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

Hi,

On 2025/10/02 1:22, Fujii Masao wrote:
> Regarding 0002:
> 
> - if (canRetryError(st->estatus))
> + if (continue_on_error || canRetryError(st->estatus))
>   {
>   if (verbose_errors)
>   commandError(st, PQresultErrorMessage(res));
>   goto error;
> 
> With this change, even non-SQL errors (e.g., connection failures) would
> satisfy the condition when --continue-on-error is set. Isn't that a problem?
> Shouldn't we also check that the error status is one that
> --continue-on-error is meant to handle?

I agree that connection failures should not be ignored even when
--continue-on-error is specified.
For now, I’m not sure if other cases would cause issues, so the updated patch
explicitly checks the connection status and emits an error message when the
connection is lost.

> 
> 
> + * Without --continue-on-error:
>   *
>   * failed (the number of failed transactions) =
>   *   'serialization_failures' (they got a serialization error and were not
>   *                             successfully retried) +
>   *   'deadlock_failures' (they got a deadlock error and were not
>   *                        successfully retried).
>   *
> + * With --continue-on-error:
> + *
> + * failed (number of failed transactions) =
> + *   'serialization_failures' + 'deadlock_failures' +
> + *   'other_sql_failures'  (they got some other SQL error; the transaction was
> + * not retried and counted as failed due to --continue-on-error).
> 
> About the comments on failed transactions: I don't think we need
> to split them into separate "with/without --continue-on-error" sections.
> How about simplifying them like this?
> 
> 
> ------------------------
> * failed (the number of failed transactions) =
> *   'serialization_failures' (they got a serialization error and were not
> *                        successfully retried) +
> *   'deadlock_failures' (they got a deadlock error and were not
> *                        successfully retried) +
> *   'other_sql_failures'  (they failed on the first try or after retries
> *                        due to a SQL error other than serialization or
> *                        deadlock; they are counted as a failed transaction
> *                        only when --continue-on-error is specified).
> ------------------------
> 
Thank you for the suggestion. I’ve updated the comments as you proposed.

> 
> * 'retried' (number of all retried transactions) =
> *   successfully retried transactions +
> *   failed transactions.
> 
> Since transactions that failed on the first try (i.e., no retries) due to
> an SQL error are not counted as 'retried', shouldn't this source comment
> be updated?

Agreed. I added "failed transactions" is actually counted when they are retied.


I've attached the updated patch v17-0002. 0003 remains unchanged.

Best regards,
Rintaro Ikeda