Re: BUG #19494: Error on transaction commit inside pipeline triggers psql's Assert

On Sun, May 31, 2026 at 12:00:01PM +0300, Alexander Lakhin wrote:
> I've found a way to trigger another assertion, but I don't think it's
> legitimate:
> --- a/src/backend/libpq/pqcomm.c
> +++ b/src/backend/libpq/pqcomm.c
> @@ -880,7 +880,7 @@ RemoveSocketFiles(void)
>  static void
>  socket_set_nonblocking(bool nonblocking)
>  {
> -       if (MyProcPort == NULL)
> +       if ((MyProcPort == NULL) || (rand() % 10 == 0))
>                 ereport(ERROR,
> (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
>                                  errmsg("there is no client connection")));

Server-side error injection. Noice.

> trigger
> psql: common.c:2055: ExecQueryAndProcessResults: Assertion
> `pset.piped_syncs > 0' failed.

This one would be in the same spirit as the others, if we cannot
really guarantee that the counters will be correct all the time we can
just more more defensive. That's an error thrown while the sync
message is processing itself, causing piped_syncs to get out of step.

> Probably there could be another way to throw an ERROR on \syncpipeline,
> but I have no good idea yet.

There is one challenge here, as far as I can see: libpq does not
really offer a way to make the difference between this thrown error
and an error that comes from a Sync, so it seems like we cannot do
much on the psql side except be more defensive? I am not sure if this
is worth the extra facility in libpq, the point would be moot in the
back branches anyway. And there is a benefit in keeping the psql code
as simple as possible, as well, so I'd tend to keep it more useful
still simpler.

> Running psql_pipeline in a loop with the above modification applied:
> for i in {1..1000}; do echo "ITERATION $i"; NO_TEMP_INSTALL=1 TESTS=psql_pipeline make -s check-tests; done
> I also observed the test hanging (at iterations 284. 543, 218) due to loss
> of synchronization between psql and postgres.

I have looked at that as well, and I don't think that this is fixable
only from the point of psql, because the error injected creates a
state where libpq's internal command queue gets out of sync regarding
what the backend has sent. The only thing that could be done is
inside libpq, as far as I can see, where we should try to detect that
the state is not synchronized anymore and fail rather than block. So
IMO, and with the error injected (which would never happen in
production in practice), the best thing I can come up with is the
attached for now.

One thing that I could see ourselves do as an extra improvement in
ExecQueryAndProcessResults() where we consume the results and check if
we're still in a busy state (some PQconsumeInput+PQisBusy). I don't
think that this should be a problem in practice, but this feels like
just hiding the real problem on the libpq side with the inconsistent
protocol state generated by the backend. I have also quickly tested
an approach based on that, unfortunately this leads to some
instability in the tests to due the async nature of the commands.

Anyway, the v3 attached passes the regression tests, handles the
pg_terminate_backend() case gracefully, handles the error case with
the error injected on backend-side a but better, and can avoid
some of the issues in the fourth case, but not all as we don't have
access to the pipe state when reaching the results do to the backend
missing up with the libpq state. Handling the 4th case more
gracefully would require some libpq changes, which may not justify the
cases we are dealing with here, at least to me. As a whole, I'd feel
that v3 is a good improvement in itself, and it addresses your
original issues and the assertions.

What do you think?
--
Michael