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

On Tue, Sep 16, 2025 at 5:34 PM Jakub Wartak
<jakub(dot)wartak(at)enterprisedb(dot)com> wrote:
>
> On Thu, Jul 24, 2025 at 5:44 AM Yugo Nagata <nagata(at)sraoss(dot)co(dot)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.

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

+$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?

+ '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.

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

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}
-----------------------------

Regards,

--
Fujii Masao