Hi Yugo, Thanks for the patch. After reviewing it, I got a few small comments:
> On Sep 25, 2025, at 15:22, Yugo Nagata <nag...@sraoss.co.jp> wrote: > > -- > Yugo Nagata <nag...@sraoss.co.jp <mailto:nag...@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/
