Re: Using PQexecQuery in pipeline mode produces unexpected Close messages

At Fri, 17 Jun 2022 20:31:50 +0200, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote in
> 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,

Yey.

> this means we'll have to go over all places that use asyncStatus to
> ensure that they all handle the new value correctly.

Sure.

> 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

# (ah, I wondered why it failed to apply..)

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

Yeah.. Actulallly I really did it carelessly.. Thanks!

> Others to think about:
>
> PQisBusy (I think no changes are needed),

Agreed.

> PQfn (I think it should accept a call in PGASYNC_PIPELINE_IDLE mode;
> fully untested in pipeline mode),

Does a PQ_PIPELINE_OFF path need that? Rather I thought that we need
Assert(!conn->asyncStatus != PGASYNC_PIPELINE_IDLE) there. But anyway
we might need a test for this path.

> PQexitPipelineMode (I think it needs to return error; needs test case),

Agreed about test case. Currently the function doesn't handle
PGASYNC_IDLE so I thought that PGASYNC_PIPELINE_IDLE also don't need a
care. If the function treats PGASYNC_PIPELINE_IDLE state as error,
the regression test fails (but I haven't examine it furtuer.)

> PQsendFlushRequest (I think it should let through; ditto).

Does that mean exit without pushing 'H' message?

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

Yeah, it was annoying that the script prints expected and result trace
separately. It looks pretty good with the patch. I don't think
there's much use of context size here.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center