Hi,
On 2025/07/10 18:17, Yugo Nagata wrote:
> On Wed, 9 Jul 2025 23:58:32 +0900
> Rintaro Ikeda <ikedarinta...@oss.nttdata.com> wrote:
>
>> Hi,
>>
>> Thank you for the kind comments.
>>
>> I've updated the previous patch.
>
> Thank you for updating the patch!
>
>>> However, if a large number of errors occur, this could result in a
>>> significant increase
>>> in stderr output during the benchmark.
>>>
>>> Users can still notice that something went wrong by checking the “number of
>>> other failures”
>>> reported after the run, and I assume that in most cases, when
>>> --continue-on-error is enabled,
>>> users aren’t particularly interested in seeing individual error messages as
>>> they happen.
>>>
>>> It’s true that seeing error messages during the benchmark might be useful
>>> in some cases, but
>>> the same could be said for serialization or deadlock errors, and that’s
>>> exactly what the
>>> --verbose-errors option is for.
>>
>>
>> I understand your concern. The condition for calling pg_log_error() was
>> modified
>> to reduce stderr output.
>> Additionally, an error message was added for cases where some clients aborted
>> while executing SQL commands, similar to other code paths that transition to
>> st->state = CSTATE_ABORTED, as shown in the example below:
>>
>> ```
>> pg_log_error("client %d aborted
>> while establishing connection", st->id);
>> st->state = CSTATE_ABORTED;
>> ```
>
> default:
> /* anything else is unexpected */
> - pg_log_error("client %d script %d aborted in command %d query
> %d: %s",
> - st->id, st->use_file, st->command, qrynum,
> - PQerrorMessage(st->con));
> + if (verbose_errors)
> + pg_log_error("client %d script %d aborted in command %d
> query %d: %s",
> + st->id, st->use_file, st->command, qrynum,
> + PQerrorMessage(st->con));
> goto error;
> }
>
> Thanks to this fix, error messages caused by SQL errors are now output only
> when
> --verbose-errors is enable. However, the comment describes the condition as
> "unexpected",
> and the message states that the client was "aborted". This does not seems
> accurate, since
> clients are not aborted due to SQL errors when --continue-on-errors is
> enabled.
>
> I think the error message should be emitted using commandError() when both
> --coneinut-on-errors and --verbose-errors are specified, like this;
>
> case PGRES_NONFATAL_ERROR:
> case PGRES_FATAL_ERROR:
> st->estatus = getSQLErrorStatus(PQresultErrorField(res,
>
> PG_DIAG_SQLSTATE));
> if (continue_on_error || canRetryError(st->estatus))
> {
> if (verbose_errors)
> commandError(st, PQerrorMessage(st->con));
> goto error;
> }
> /* fall through */
>
> In addition, the error message in the "default" case should be shown
> regardless
> of the --verbose-errors since it represents an unexpected situation and should
> always reported.
>
> Finally, I believe this fix should be included in patch 0001 rather than 0003,
> as it would be a part of the implementation of --continiue-on-error.
>
>
> As of 0003:
>
> + {
> + pg_log_error("client %d aborted while executing SQL
> commands", st->id);
> st->state = CSTATE_ABORTED;
> + }
> break;
>
> I understand that the patch is not directly related to --continue-on-error,
> similar to 0002,
> and that it aims to improve the error message to indicate that the client was
> aborted due to
> some error during readCommandResponse().
>
> However, this message doesn't seem entirely accurate, since the error is not
> always caused
> by an SQL command failure itself. For example, it could also be due to a
> failure of the \gset
> meta-command.
>
> In addition, this fix causes error messages to be emitted twice. For example,
> if \gset fails,
> the following similar messages are printed:
>
> pgbench: error: client 0 script 0 command 0 query 0: expected one row, got 0
> pgbench: error: client 0 aborted while executing SQL commands
>
> Even worse, if an unexpected error occurs in readCommandResponse() (i.e., the
> default case),
> the following messages are emitted, both indicating that the client was
> aborted;
>
> pgbench: error: client 0 script 0 aborted in command ... query ...
> pgbench: error: client 0 aborted while executing SQL commands
>
> I feel this is a bit redundant.
>
> Therefore, if we are to improve these messages to indicate explicitly that
> the client
> was aborted, I would suggest modifying the error messages in
> readCommandResponse() rather
> than adding a new one in advanceConnectionState().
>
> I've attached patch 0003 incorporating my suggestion. What do you think?
Thank you very much for the updated patch!
I reviewed 0003 and it looks great - the error message become easier to
understand.
I noticed one small thing I’d like to discuss. I'm not sure that users clearly
understand which aborted in the following error message, the client or the
script.
> pgbench: error: client 0 script 0 aborted in command ... query ...
Since the code path always results in a client abort, I wonder if the following
message might be clearer:
> pgbench: error: client 0 aborted in script 0 command ... query ...
Regards,
Rintaro Ikeda
