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
