Re: Suggestion to add --continue-client-on-abort option to pgbench - Mailing list pgsql-hackers

On Fri, 19 Sep 2025 11:43:28 +0900
Fujii Masao <masao.fujii@gmail.com> wrote:

> On Thu, Sep 18, 2025 at 4:20 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:
> >
> > On Thu, 18 Sep 2025 14:37:29 +0900
> > Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > > On Thu, Sep 18, 2025 at 10:22 AM Yugo Nagata <nagata@sraoss.co.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...

I don’t have a strong opinion on this, but I think TSTATUS_* is slightly
related to PQTRANS_*, since getTransactionStatus() determines the TSTATUS
value based on PQTRANS. There is no one-to-one relationship, of course,
but it is more related than ESTATUS_OTHER_SQL_ERROR, which is entirely
separate.

> 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.

+1

I personally don't care if ommiting this change, but I would like to wait 
for Ikeda-san's response because he is the author of these two patches.

> 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?

"qrynum" is the index of SQL queries combined by "\;", but reporting it
in \gset errors is almost useless, since \gset can only be applied to the
last query of a compound query. So I think it’s fine to commit this.

That said, it might still be useful for debugging when an internal error like 
the following occurs (mainly for developers rather than users):

      /* internal error */
      commandFailed(st, cmd, psprintf("error storing into variable %s", varname));

For that case, I’d be fine with adding information like this:

      /* internal error */
      commandFailed(st, cmd, psprintf("error storing into variable %s, at query %d", varname, qrynum));

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>