Re: Using PQexecQuery in pipeline mode produces unexpected Close messages

Thanks for the further testing scenario.

At Wed, 29 Jun 2022 14:09:17 +0200, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote in
> So I wrote some more test scenarios for this, and as I wrote in some
> other thread, I realized that there are more problems than just some
> NOTICE trouble. For instance, if you send a query, then read the result
> but not the terminating NULL then send another query, everything gets
> confused; the next thing you'll read is the result for the second query,
> without having read the NULL that terminates the results of the first
> query. Any application that expects the usual flow of results will be
> confused. Kyotaro's patch to add PIPELINE_IDLE fixes this bit too, as
> far as I can tell.
>
> However, another problem case, not fixed by PIPELINE_IDLE, occurs if you
> exit pipeline mode after PQsendQuery() and then immediately use
> PQexec(). The CloseComplete will be received at the wrong time, and a
> notice is emitted nevertheless.

Mmm. My patch moves the point of failure of the scenario a bit but
still a little short. However, as my understanding, it seems like the
task of the PQpipelineSync()-PQgetResult() pair to consume the
CloseComplete. If Iinserted PQpipelineSync() just after PQsendQuery()
and called PQgetResult() for PGRES_PIPELINE_SYNC before
PQexitPipelineMode(), the out-of-sync CloseComplete is not seen in the
scenario. But if it is right, I'd like to complain about the
obscure-but-stiff protocol of pipleline mode..

> I spent a lot of time trying to understand how to fix this last bit, and
> the solution I came up with is that PQsendQuery() must add a second
> entry to the command queue after the PGQUERY_EXTENDED one, to match the
> CloseComplete message being expected; with that entry in the queue,
> PQgetResult will really only get to IDLE mode after the Close has been
> seen, which is what we want. I named it PGQUERY_CLOSE.
>
> Sadly, some hacks are needed to make this work fully:
>
> 1. the client is never expecting that PQgetResult() would return
> anything for the CloseComplete, so something needs to consume the
> CloseComplete silently (including the queue entry for it) when it is
> received; I chose to do this directly in pqParseInput3. I tried to
> make PQgetResult itself do it, but it became a pile of hacks until I
> was no longer sure what was going on. Putting it in fe-protocol3.c
> ends up a lot cleaner. However, we still need PQgetResult to invoke
> parseInput() at the point where Close is expected.
>
> 2. if an error occurs while executing the query, the CloseComplete will
> of course never arrive, so we need to erase it from the queue
> silently if we're returning an error.
>
> I toyed with the idea of having parseInput() produce a PGresult that
> encodes the Close message, and have PQgetResult consume and discard
> that, then read some further message to have something to return. But
> it seemed inefficient and equally ugly and I'm not sure that flow
> control is any simpler.
>
> I think another possibility would be to make PQexitPipelineMode
> responsible for /something/, but I'm not sure what that would be.
> Checking the queue and seeing if the next message is CloseComplete, then
> eating that message before exiting pipeline mode? That seems way too
> magical. I didn't attempt this.
>
> I attach a patch series that implements the proposed fix (again for
> REL_14_STABLE) in steps, to make it easy to read. I intend to squash
> the whole lot into a single commit before pushing. But while writing
> this email it occurred to me that I need to add at least one more test,
> to receive a WARNING while waiting for CloseComplete. AFAICT it should
> work, but better make sure.
>
> I produced pipeline_idle.trace file by running the test in the fully

By the perl script doesn't produce the trace file since the list in
$cmptrace line doesn't contain pipleline_idle..

> fixed tree, then used it to verify that all tests fail in different ways
> when run without the fixes. The first fix with PIPELINE_IDLE fixes some
> of these failures, and the PGQUERY_CLOSE one fixes the remaining one.
> Reading the trace file, it looks correct to me.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center