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

Hi Yugo,

Thanks for the patch. After reviewing it, I got a few small comments:

> On Sep 25, 2025, at 15:22, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> --
> Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp <mailto:nagata(at)sraoss(dot)co(dot)jp>>
> <v13-0003-Improve-error-messages-for-errors-that-cause-cli.patch><v13-0002-Add-continue-on-error-option.patch><v13-0001-Fix-assertion-failure-and-verbose-messages-in-pi.patch>

1 - 0001
```
@@ -3265,6 +3271,7 @@ readCommandResponse(CState *st, MetaCommand meta, char *varprefix)
PGresult *res;
PGresult *next_res;
int qrynum = 0;
+ char *errmsg;
```

I think we should initialize errmsg to NULL. Compiler won’t auto initialize a local variable. If it happens to not enter the while loop, then errmsg will hold a random value, then pg_free(errmsg) will have trouble.

2 - 0002
```
+ <para>
+ Allows clients to continue their run even if an SQL statement fails due to
+ errors other than serialization or deadlock. Unlike serialization and deadlock
+ failures, clients do not retry the same transactions but start new transaction.
+ This option is useful when your custom script may raise errors due to some
+ reason like unique constraints violation. Without this option, the client is
+ aborted after such errors.
+ </para>
```

A few nit suggestions:

* “continue their run” => “continue running”
* “clients to not retry the same transactions but start new transaction” => “clients do not retry the same transaction but start a new transaction instead"
* “due to some reason like” => “for reasons such as"

3 - 0002
```
+ * Without --continue-on-error:
* failed (the number of failed transactions) =
```

Maybe add an empty line after “without” line.

4 - 0002
```
+ * When --continue-on-error is specified:
+ *
+ * failed (number of failed transactions) =
```

Maybe change to “With ---continue-on-error”, which sounds consistent with the previous “without”.

5 - 0002
```
+ int64 other_sql_failures; /* number of failed transactions for
+ * reasons other than
+ * serialization/deadlock failure, which
+ * is counted if --continue-on-error is
+ * specified */
```

How about rename this variable to “sql_errors”, which reflects to the new option name.

6 - 0002
```
@@ -4571,6 +4594,8 @@ getResultString(bool skipped, EStatus estatus)
return "serialization";
case ESTATUS_DEADLOCK_ERROR:
return "deadlock";
+ case ESTATUS_OTHER_SQL_ERROR:
+ return "other”;
```

I think this can just return “error”. I checked where this function is called, there will not be other words such as “error” appended.

7 - 0002
```
/* it can be non-zero only if max_tries is not equal to one */
@@ -6569,6 +6602,10 @@ printResults(StatsData *total,
sstats->deadlock_failures,
(100.0 * sstats->deadlock_failures /
script_total_cnt));
+ printf(" - number of other failures: " INT64_FORMAT " (%.3f%%)\n",
+ sstats->other_sql_failures,
+ (100.0 * sstats->other_sql_failures /
+ script_total_cnt));
```

Do we only want to print this number when “—continue-on-error” is given?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/