On 2022-Jun-16, Kyotaro Horiguchi wrote:
> The attached is a crude patch to separate the state for PIPELINE-IDLE
> from PGASYNC_IDLE. I haven't found a better way..
Ah, yeah, this might be a way to fix this.
Something very similar to a PIPELINE_IDLE mode was present in Craig's
initial patch for pipeline mode. However, I fought very hard to remove
it, because it seemed to me that failing to handle it correctly
everywhere would lead to more bugs than not having it. (Indeed, there
were some.)
However, I see now that your patch would not only fix this bug, but also
let us remove the ugly "notionally not-idle" bit in fe-protocol3.c,
which makes me ecstatic. So let's push forward with this. However,
this means we'll have to go over all places that use asyncStatus to
ensure that they all handle the new value correctly.
I did found one bug in your patch: in the switch for asyncStatus in
PQsendQueryStart, you introduce a new error message. With the current
tests, that never fires, which is telling us that our coverage is not
complete. But with the right sequence of libpq calls, which the
attached adds (note that it's for REL_14_STABLE), that can be hit, and
it's easy to see that throwing an error there is a mistake. The right
action to take there is to let the action through.
Others to think about:
PQisBusy (I think no changes are needed),
PQfn (I think it should accept a call in PGASYNC_PIPELINE_IDLE mode;
fully untested in pipeline mode),
PQexitPipelineMode (I think it needs to return error; needs test case),
PQsendFlushRequest (I think it should let through; ditto).
I also attach a patch to make the test suite use Test::Differences, if
available. It makes the diffs of the traces much easier to read, when
they fail. (I wish for a simple way to set the context size, but that
would need a shim routine that I'm currently too lazy to write.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
