Re: Suggestion to add --continue-client-on-abort option to pgbench - Mailing list pgsql-hackers

Hi,

Thank you for the kind comments.

I've updated the previous patch.

Below is a summary of the changes:
1. The code path and documentation have been corrected based on your feedback.
2. The following message is now suppressed by default. Instead, an error message
is added when a client aborts during SQL execution. (v6-0003-Suppress-xxx.patch)

```
                if (verbose_errors)
                    pg_log_error("client %d script %d aborted in command %d query %d: %s",
                                 st->id, st->use_file, st->command, qrynum,
                                 PQerrorMessage(st->con));
```


On 2025/07/04 22:01, Hayato Kuroda (Fujitsu) wrote:

>>> Could I confirm what you mean by "start new one"?
>>>
>>> In the current pgbench, when a query raises an error (a deadlock or
>>> serialization failure), it can be retried using the same random state.
>>> This typically means the query will be retried with the same parameter values.
>>>
>>> On the other hand, when the query ultimately fails (possibly after some retries),
>>> the transaction is marked as a "failure", and the next transaction starts with a
>>> new random state (i.e., with new parameter values).
>>>
>>> Therefore, if a query fails due to a unique constraint violation and is retried
>>> with the same parameters, it will keep failing on each retry.
>>
>> Thank you for your explanation. I understand it as you described. I've also
>> attached a schematic diagram of the state machine. I hope it will help clarify
>> the behavior of pgbench. Red arrows represent the transition of state when SQL
>> command fails and --continue-on-error option is specified.
>
> Thanks for the diagram, it's quite helpful. Let me share my understanding and opinion.
>
> The terminology "retry" is being used for the transition CSTATE_ERROR->CSTATE_RETRY,
> and here the random state would be restored to be the begining:
>
> ```
>                 /*
>                  * Reset the random state as they were at the beginning of the
>                  * transaction.
>                  */
>                 st->cs_func_rs = st->random_state;
> ```
>
> In --continue-on-error case, the transaction CSTATE_WAIT_RESULT->CSTATE_ERROR
> can happen even the reason of failure is not serialization and deadlock.
> Ultimately the pass will reach ...->CSTATE_END_TX->CSTATE_CHOOSE_SCRIPT, the
> beginning of the state machine. cs_func_rs is not overwritten in the route so
> that different random value would be generated, or even another script may be
> chosen. Is it correct?

Yes, I believe that’s correct.

>
> 01.
> ```
> $ git am ../patches/pgbench/v5-0001-Add-continue-on-error-opt
> ion-to-pgbench.patch
> Applying: When the option is set, client rolls back the failed transaction and...
> .git/rebase-apply/patch:65: trailing whitespace.
>    <literal>serialization</literal>, <literal>deadlock</literal>, or
> .git/rebase-apply/patch:139: trailing whitespace.
>    <option>--max-tries</option> option is not equal to 1 and
> warning: 2 lines add whitespace errors.
> ```
>
> I got warnings when I applied the patch. Please fix it.

It's been fixed.

>
> 02.
> ```
> +        *       'serialization_failures' + 'deadlock_failures' +
> +        *   'other_sql_failures' (they got a error when continue-on-error option
> ```
> The first line has the tab, but it should be normal blank.

I hadn't noticed it. It's fixed.


> 03.
> ```
> +                               else if (continue_on_error | canRetryError(st->estatus))
> ```
>
> I feel "|" should be "||".

Thank you for pointing out. Fixed it.

> 04.
> ```
>      <term><replaceable>retries</replaceable></term>
>      <listitem>
>       <para>
>        number of retries after serialization or deadlock errors
>        (zero unless <option>--max-tries</option> is not equal to one)
>       </para>
>      </listitem>
> ```
>
> To confirm; --continue-on-error won't be counted here because it is not "retry",
> in other words, it does not reach CSTATE_RETRY, right?

Yes. I agree with Nagata-san [1] — --continue-on-error is not considered a
"retry" because it doesn't reach CSTATE_RETRY.


On 2025/07/05 0:03, Yugo Nagata wrote:
>>>              case PGRES_NONFATAL_ERROR:
>>>              case PGRES_FATAL_ERROR:
>>>                  st->estatus = getSQLErrorStatus(PQresultErrorField(res,
>>>                                                                     PG_DIAG_SQLSTATE));
>>>                  if (canRetryError(st->estatus))
>>>                  {
>>>                      if (verbose_errors)
>>>                          commandError(st, PQerrorMessage(st->con));
>>>                      goto error;
>>>                  }
>>>                  /* fall through */
>>>
>>>              default:
>>>                  /* anything else is unexpected */
>>>                  pg_log_error("client %d script %d aborted in command %d query %d: %s",
>>>                               st->id, st->use_file, st->command, qrynum,
>>>                               PQerrorMessage(st->con));
>>>                  goto error;
>>>          }
>>>
>>> When an SQL error other than a serialization or deadlock error occurs, an error message is
>>> output via pg_log_error in this code path. However, I think this should be reported only
>>> when verbose_errors is set, similar to how serialization and deadlock errors are handled when
>>> --continue-on-error is enabled
>>
>> I think the error message logged via pg_log_error is useful when verbose_errors
>> is not specified, because it informs users that the client has exited. Without
>> it, users may not notice that something went wrong.
>
> However, if a large number of errors occur, this could result in a significant increase
> in stderr output during the benchmark.
>
> Users can still notice that something went wrong by checking the “number of other failures”
> reported after the run, and I assume that in most cases, when --continue-on-error is enabled,
> users aren’t particularly interested in seeing individual error messages as they happen.
>
> It’s true that seeing error messages during the benchmark might be useful in some cases, but
> the same could be said for serialization or deadlock errors, and that’s exactly what the
> --verbose-errors option is for.


I understand your concern. The condition for calling pg_log_error() was modified
to reduce stderr output.
Additionally, an error message was added for cases where some clients aborted
while executing SQL commands, similar to other code paths that transition to
st->state = CSTATE_ABORTED, as shown in the example below:

```
                        pg_log_error("client %d aborted while establishing connection", st->id);
                        st->state = CSTATE_ABORTED;
```


> Here are some comments on the patch.
>
> (1)
>
>                 }
> -               else if (canRetryError(st->estatus))
> +               else if (continue_on_error | canRetryError(st->estatus))
>                     st->state = CSTATE_ERROR;
>                 else
>                     st->state = CSTATE_ABORTED;
>
> Due to this change, when --continue-on-error is enabled, st->state is set to
> CSTATE_ERROR regardless of the type of error returned by readCommandResponse.
> When the error is not ESTATUS_OTHER_SQL_ERROR, e.g. ESTATUS_META_COMMAND_ERROR
> due to a failure of \gset with query returning more the one row.
>
> Therefore, this should be like:
>
>                else if ((st->estatus == ESTATUS_OTHER_SQL_ERROR  && continue_on_error) ||
>                          canRetryError(st->estatus))
>

Thanks for pointing that out — I’ve corrected it.


> (2)
>
> +          "  --continue-on-error      continue processing transations after a trasaction fails\n"
>
> "trasaction" is a typo　and including "transaction" twice looks a bit redundant.
> Instead using the word "transaction", how about:
>
>  "--continue-on-error continue running after an SQL error" ?
>
> This version is shorter, avoids repetition, and describes well the actual behavior when
> SQL statements fail.

Fixed it.

> (3)
>
> -    * A failed transaction is defined as unsuccessfully retried transactions.
> +    * A failed transaction is defined as unsuccessfully retried transactions
> +    * unless continue-on-error option is specified.
>      * It can be one of two types:
>      *
>      * failed (the number of failed transactions) =
> @@ -411,6 +412,12 @@ typedef struct StatsData
>      *   'deadlock_failures' (they got a deadlock error and were not
>      *                        successfully retried).
>      *
> +    * When continue-on-error option is specified,
> +    * failed (the number of failed transactions) =
> +    *   'serialization_failures' + 'deadlock_failures' +
> +    *   'other_sql_failures' (they got a error when continue-on-error option
> +    *                         was specified).
> +    *
>
> To explain explicitly that there are two definitions of failed transactions
> depending on the situation, how about:
>
> """
>  A failed transaction is counted differently depending on whether
>  the --continue-on-error option is specified.
>
>  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).
>
>  When --continue-on-error is specified:
>
>  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).
> """

Thank you for your suggestion. I modified it accordingly.


> (4)
> +   int64       other_sql_failures; /* number of failed transactions for
> +                                    * reasons other than
> +                                    * serialization/deadlock failure , which
> +                                    * is enabled if --continue-on-error is
> +                                    * used */
>
> Is "counted" is more proper than "enabled" here?

Fixed.


>
> Af for the documentations:
> (5)
>    The next line reports the number of failed transactions due to
> -  serialization or deadlock errors (see <xref linkend="failures-and-retries"/>
> -  for more information).
> +  serialization or deadlock errors by default (see
> +  <xref linkend="failures-and-retries"/> for more information).
>
> Would it be more readable to simply say:
> "The next line reports the number of failed transactions (see ... for more information),
> since definition of "failed transaction" has become a bit messy?
>

I fixed it to the simple explanation.

> (6)
>     connection with the database server was lost or the end of script was reached
>     without completing the last transaction. In addition, if execution of an SQL
>     or meta command fails for reasons other than serialization or deadlock errors,
> -   the client is aborted. Otherwise, if an SQL command fails with serialization or
> -   deadlock errors, the client is not aborted. In such cases, the current
> -   transaction is rolled back, which also includes setting the client variables
> -   as they were before the run of this transaction (it is assumed that one
> -   transaction script contains only one transaction; see
> -   <xref linkend="transactions-and-scripts"/> for more information).
> +   the client is aborted by default. However, if the --continue-on-error option
> +   is specified, the client does not abort and proceeds to the next transaction
> +   regardless of the error. This case is reported as other failures in the output.
> +   Otherwise, if an SQL command fails with serialization or deadlock errors, the
> +   client is not aborted. In such cases, the current transaction is rolled back,
> +   which also includes setting the client variables as they were before the run
> +   of this transaction (it is assumed that one transaction script contains only
> +   one transaction; see <xref linkend="transactions-and-scripts"/> for more information).
>
> To emphasize the default behavior, I wonder it would be better to move "by default"
> to the beginning of the statements; like
>
>  "By default, if execution of an SQL or meta command fails for reasons other than
>  serialization or deadlock errors, the client is aborted."
>
> How about quoting "other failures"? like:
>
>  "These cases are reported as "other failures" in the output."
>
> Also, I feel the meaning of "Otherwise" has becomes somewhat unclear since the
> explanation of --continue-on-error was added between the sentences So, how about
> clarifying that "the clients are not aborted due to serializable/deadlock even without
> --continue-on-error".  For example;
>
>  "On contrast, if an SQL command fails with serialization or deadlock errors, the
>   client is not aborted even without  <option>--continue-on-error</option>.
>   Instead, the current transaction is rolled back, which also includes setting
>   the client variables as they were before the run of this transaction
>   (it is assumed that one transaction script contains only
>    one transaction; see <xref linkend="transactions-and-scripts"/> for more information)."
>

I've modified according to your suggestion.

> (7)
>     The main report contains the number of failed transactions. If the
> -   <option>--max-tries</option> option is not equal to 1, the main report also
> +   <option>--max-tries</option> option is not equal to 1 and
> +   <option>--continue-on-error</option> is not specified, the main report also
>     contains statistics related to retries: the total number of retried
>
> Is that true?
> The retreis statitics would be included even without --continue-on-error.

That was wrong. I corrected it.


[1]
https://www.postgresql.org/message-id/20250705002239.27e6e5a4ba22c047ac2fa16a%40sraoss.co.jp

Regards,
Rintaro Ikeda