Thread: 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@alvh.no-ip.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

On 2022-Jul-04, Kyotaro Horiguchi wrote:

> At Wed, 29 Jun 2022 14:09:17 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 

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

Yeah, if you introduce PQpipelineSync then I think it'll work okay, but
my point here was to make it work without requiring that; that's why I
wrote the test to use PQsendFlushRequest instead.

BTW I patch for the problem with uniqviol also (not fixed by v7).  I'll
send an updated patch in a little while.

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

Ouch, of course, thanks for noticing.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2022-Jul-04, Alvaro Herrera wrote:

> BTW I patch for the problem with uniqviol also (not fixed by v7).  I'll
> send an updated patch in a little while.

Here it is.  I ran "libpq_pipeline uniqviol" in a tight loop a few
thousand times and didn't get any error.  Before these fixes, it would
fail in half a dozen iterations.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

I have pushed this to all three branches.  Thanks!

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
                http://smylers.hates-software.com/2005/09/08/1995c749.html