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

Dear Nagata-san,

> > > 2. The exit-on-abort option and continue-on-error option are mutually
> exclusive.
> > > Therefore, I've updated the patch to throw a FATAL error when two options
> are
> > > set simultaneously. Corresponding explanation was also added.
> 
> I don't think that's right since "abort" and "error" are different concept in pgbench.
> (Here, "abort" refers to the termination of a client, not a transaction abort.)
> 
> The --exit-on-abort option forces to exit pgbench immediately when any client is
> aborted
> due to some error. When the --continue-on-error option is not set, SQL errors
> other than
> deadlock or serialization error cause a client to be aborted. On the other hand,
> when the option
> is set, clients are not aborted due to any SQL errors; instead they continue to run
> after them.
> However, clients can still be aborted for other reasons, such as connection
> failures or
> meta-command errors (e.g., \set x 1/0). In these cases, the --exit-on-abort option
> remains
> useful even when --continue-on-error is enabled.

To clarify: another approach is that allow --continue-on-error option to continue
running even when clients meet such errors. Which one is better?

> > 02. patch separation
> > How about separating the patch series like:
> >
> > 0001 - contains option handling and retry part, and documentation
> > 0002 - contains accumulation/reporting part
> > 0003 - contains tests.
> >
> > I hope above style is more helpful for reviewers.
> 
> I'm not sure whether it's necessary to split the patch, as the change doesn't seem
> very
> complex. However, the current separation appears inconsistent. For example,
> patch 0001
> modifies canRetryError(), but patch 0002 reverts that change, and so on.

Either way is fine for me if they are changed from the current method.

> 
> >
> > 04. documentation
> > ```
> > +        Client rolls back the failed transaction and starts a new one when its
> > +        transaction fails due to the reason other than the deadlock and
> > +        serialization failure. This allows all clients specified with -c option
> > +        to continuously apply load to the server, even if some transactions
> fail.
> > ```
> >
> > I feel the description contains bit redundant part and misses the default
> behavior.
> > How about:
> > ```
> >        <para>
> >         Clients survive when their transactions are aborted, and they continue
> >         their run. Without the option, clients exit when transactions they run
> >         are aborted.
> >        </para>
> >        <para>
> >         Note that serialization failures or deadlock failures do not abort the
> >         client, so they are not affected by this option.
> >         See <xref linkend="failures-and-retries"/> for more information.
> >        </para>
> > ```
> 
> I think we can make it clearer as follows:

I do not have confident for English, native speaker is needed....

> > 06. usage()
> > Added line is too long. According to program_help_ok(), the output by help
> should
> > be less than 80.
> 
> +1

FYI - I posted a patch which adds the test. You can apply and confirm how the function says.

[1]: https://www.postgresql.org/message-id/OSCPR01MB1496610451F5896375B2562E6F56BA%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED