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

On Thu, Sep 18, 2025 at 4:20 PM Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> On Thu, 18 Sep 2025 14:37:29 +0900
> Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> > On Thu, Sep 18, 2025 at 10:22 AM Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> > > That makes sense. How about rewriting this like:
> > >
> > > However, if the --continue-on-error option is specified and the error occurs in
> > > an SQL command, the client does not abort and proceeds to the next
> > > transaction regardless of the error. These cases are reported as "other failures"
> > > in the output. Note that if the error occurs in a meta-command, the client will
> > > still abort even when this option is specified.
> >
> > How about phrasing it like this, based on your version?
> >
> > ----------------------------
> > A client's run is aborted in case of a serious error; for example, the
> > connection with the database server was lost or the end of script was reached
> > without completing the last transaction. The client also aborts
> > if a meta-command fails, or if an SQL command fails for reasons other than
> > serialization or deadlock errors when --continue-on-error is not specified.
> > With --continue-on-error, the client does not abort on such SQL errors
> > and instead proceeds to the next transaction. These cases are reported
> > as "other failures" in the output. If the error occurs in a meta-command,
> > however, the client still aborts even when this option is specified.
> > ----------------------------
>
> I'm fine with that. This version is clearer.

Thanks for checking!

Also I'd like to share the review comments for 0002 and 0003.

Regarding 0002:

- TSTATUS_OTHER_ERROR,
+ TSTATUS_UNKNOWN_ERROR,

You did this rename to avoid confusion with other_sql_errors.
I see the intention, but I'm not sure if this concern is really valid
and if the rename adds much value. Also, TSTATUS_UNKNOWN_ERROR
might be mistakenly assumed to be related to PQTRANS_UNKNOWN,
even though they aren't related...

But if we agree with this change, I think it should be folded into 0001,
since there's no strong reason to keep it separate.

Regarding 0003:

- pg_log_error("client %d script %d command %d query %d: expected one
row, got %d",
- st->id, st->use_file, st->command, qrynum, 0);
+ commandFailed(st, "gset", psprintf("expected one row, got %d", 0));

The change to use commandFailed() seems to remove
the "query %d" detail that the current pg_log_error() call reports.
Is it OK to lose that information?

Regards,

--
Fujii Masao