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

On Tue, Sep 30, 2025 at 3:17 PM Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> On Tue, 30 Sep 2025 13:46:11 +0900
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> > On Tue, Sep 30, 2025 at 10:24 AM Yugo Nagata <nagata(at)sraoss(dot)co(dot)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