Re: Suggestion to add --continue-client-on-abort option to pgbench - Mailing list pgsql-hackers
| From | Chao Li |
|---|---|
| Subject | Re: Suggestion to add --continue-client-on-abort option to pgbench |
| Date | |
| Msg-id | E7609E33-5847-45EB-A4F7-615348FC12BA@gmail.com Whole thread Raw |
| In response to | Re: Suggestion to add --continue-client-on-abort option to pgbench (Yugo Nagata <nagata@sraoss.co.jp>) |
| Responses |
Re: Suggestion to add --continue-client-on-abort option to pgbench
|
| List | pgsql-hackers |
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>
<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/
HighGo Software Co., Ltd.
https://www.highgo.com/
pgsql-hackers by date: