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@sraoss.co.jp> wrote:
> 
> -- 
> Yugo Nagata <nagata@sraoss.co.jp <mailto:nagata@sraoss.co.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/