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

On Wed, 12 Nov 2025 01:47:37 +0900
Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Tue, Nov 11, 2025 at 11:49 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> > I just did more tests on both pipeline mode and non-pipeline mode, I think the main purpose of discardAvailableResults() is to drain results for pipeline mode. In non-pipeline mode, a NULL res indicates no more result to read; while in pipeline mode, when a pipeline is aborted, either a valid result or NULL could still be returned, thus we need to wait until pipeline state switch to PQ_PIPELINE_OK. From this perspective, the current inline comment is correct, but I feel it’s not clear enough.
>
> Thanks for working on this!
>
> After reconsidering, I think the main goal here is to determine whether
> the error causes a connection failure after it occurs.
>
> If we can read and discard results without PQstatus() becoming CONNECTION_BAD
> either until the end (in non-pipeline mode) or until the first sync point
> after an error (in pipeline mode), that means the connection is still alive,
> and processing can continue when --continue-on-error is specified.
>
> The current function comments don’t mention this purpose enough,
> so seems they should be updated to clarify that.

I agree that the goal of this function is to discard results until the point
where a connection failure can be detected. When the socket reaches EOF,
PQgetResult() returns PGRES_FATAL_ERROR to report it, followed by NULL.
However, in an aborted pipeline, several NULLs following each PGRES_PIPELINE_ABORTED
may be returned before that, so we need to discard those NULLs beforehand.

Considering this, the function name "discardAvailableResults" might be a bit misleading,
since it doesn’t actually discard all available results. How about renaming it to something
like "discardForErrorStatusCheck" (a bit long, though)?

Related to this, I doubt the necessity of calling this function after the error: label in
readCommandResponse(). If the error is retriable, all results will be discarded later by
discardUntilSync(). If it’s not retriable, the thread will immediately exit and the connection
will be abandoned, so discarding results here seems unnecessary.

If discardAvailableResults() is unnecessary here, we could embed its logic into
getSQLErrorStatus() instead of leaving it as a separate function.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>