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

From: Rintaro Ikeda <ikedarintarof(at)oss(dot)nttdata(dot)com>
To: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
Cc: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, Jakub Wartak <jakub(dot)wartak(at)enterprisedb(dot)com>, "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com>, "slpmcf(at)gmail(dot)com" <slpmcf(at)gmail(dot)com>, "boekewurm+postgres(at)gmail(dot)com" <boekewurm+postgres(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Srinath Reddy Sadipiralla <srinath2133(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>
Subject: Re: Suggestion to add --continue-client-on-abort option to pgbench
Date: 2025-10-19 13:12:37
Message-ID: 0e09dba1-ad57-4bc5-9a05-8f0a08b59119@oss.nttdata.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v17-0002-pgbench-Add-continue-on-error-option.patch text/plain 15.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Mihail Nikalayeu 2025-10-19 13:41:00 isolation tester limitation in case of multiple injection points in a single command
Previous Message Álvaro Herrera 2025-10-19 11:32:13 Re: Accessing an invalid pointer in BufferManagerRelation structure