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

On Tue, Sep 30, 2025 at 3:17 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> On Tue, 30 Sep 2025 13:46:11 +0900
> Fujii Masao <masao.fujii@gmail.com> wrote:
>
> > On Tue, Sep 30, 2025 at 10:24 AM Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > > Fujii-san, thank you for committing the patch that fixes the assertion failure.
> > > I've attached the remaining patches so that cfbot stays green.
> >
> > Thanks for reattaching the patches!
> >
> > For 0001, after reading the docs on PQresultErrorMessage(), I wonder if it would
> > be better to just use that to get the error message. Thought?
>
> Thank you for your suggestion.
>
> I agree that it is better to use PQresultErrorMessage().
> I had overlooked the existence of this interface.
>
> I've attached the updated patches.

Thanks for updating the patches! I've pushed 0001.

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?


+ * 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).
------------------------


* '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?

Regards,

-- 
Fujii Masao