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

On Thu, 18 Sep 2025 01:52:46 +0900
Fujii Masao <masao.fujii@gmail.com> wrote:

> On Tue, Sep 16, 2025 at 5:34 PM Jakub Wartak
> <jakub.wartak@enterprisedb.com> wrote:
> >
> > On Thu, Jul 24, 2025 at 5:44 AM Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >
> > > >
> > > > I believe the patches implement the expected behavior, include appropriste doc and test
> > > > modification, are in good shape overall, so if there are no objections,
> > > > I'll mark this as Read-for-Committer.
> > >
> > > I've updated the CF status to Ready for Committer.
> 
> Since this patch is marked as ready for committer, I've started reviewing it.
> The patch basically looks good to me.
> 
> 
> +   the client is aborted. However, if the --continue-on-error option
> is specified,
> 
> "--continue-on-error" should be enclosed in <option> tags.

+1

> +   without completing the last transaction. By default, if execution of an SQL
>     or meta command fails for reasons other than serialization or
> deadlock errors,
> <snip>
> +   the client is aborted. However, if the --continue-on-error option
> is specified,
> +   the client does not abort and proceeds to the next transaction regardless of
> +   the error. These cases are reported as "other failures" in the output.
> 
> This explanation can be read as if --continue-on-error allows the client to
> proceed to the next transaction even when mata command (not SQL) fails,
> but that is not correct, right? If so, the description should be updated to
> make it clear that only SQL errors are affected, while meta command failures
> are not.

That makes sense. How about rewriting this like:

 However, if the --continue-on-error option is specified and the error occurs in
 an SQL command, the client does not abort and proceeds to the next
 transaction regardless of the error. These cases are reported as "other failures"
 in the output. Note that if the error occurs in a meta-command, the client will
 still abort even when this option is specified.


> +$node->pgbench(
> + '-t 10 --continue-on-error --failures-detailed',
> 
> Isn't it better to specify also -n option to skip unnecessary VACUUM and
> speed the test up?

+1
 
> + 'test --continue-on-error',
> + {
> + '002_continue_on_error' => q{
> 
> Regarding the test file name, perhaps 001 would be a better prefix than 002,
> since other tests in 001_pgbench_with_server.pl use 001 as the prefix.

Right. This filename is shown in the “transaction type:” field of the results
when the test fails, so it should be aligned with the test file name.

> + insert into unique_table values 0;
> 
> This INSERT causes a syntax error. Was this intentional? If the intention was
> to test unique constraint violations, it should instead be
> INSERT INTO unique_table VALUES (0);.

This was clearly unintentional. I happened to overlook it during my review.

> To further improve the test, it might also be useful to mix successful and
> failed transactions in the --continue-on-error case. For example,
> the following change would result in one successful transaction and
> nine failures:
> 
> -----------------------------
>  $node->safe_psql('postgres',
> -       'CREATE TABLE unique_table(i int unique);' . 'INSERT INTO
> unique_table VALUES (0);');
> +       'CREATE TABLE unique_table(i int unique);');
> 
>  $node->pgbench(
>         '-t 10 --continue-on-error --failures-detailed',
>         0,
>         [
> -               qr{processed: 0/10\b},
> -               qr{other failures: 10\b}
> +               qr{processed: 1/10\b},
> +               qr{other failures: 9\b}
> -----------------------------

+1
This makes the purpose of the test clearer.

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>