From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@lists.postgresql.org, Craig Ringer <craig.ringer@enterprisedb.com>
Cc: Matthieu Garrigues <matthieu.garrigues@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Ibrar Ahmed <ibrar.ahmad@gmail.com>
Subject: Re: [HACKERS] PATCH: Batch/pipelining support for libpq
Date: Thu, 11 Mar 2021 13:49:02 -0500
Message-ID: <2311038.1615488542@sss.pgh.pa.us> (raw)
In-Reply-To: <20210311160013.GA13683@alvherre.pgsql>
References: <20210311160013.GA13683@alvherre.pgsql>
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> [ v35-libpq-pipeline.patch ]
I think the changes in pqParseInput3() are broken. You should have
kept the else-structure as-is and inserted the check for "not really
idle" inside the else-clause that reports an error. As it stands,
after successfully processing an asynchronously-received error or
ParameterStatus message, the added code will cause us to return without
advancing inStart, creating an infinite loop of reprocessing that message.
It's possible that we should redefine the way things happen so that if
we're waiting for another pipeline event, we should hold off processing
of async error & ParameterStatus; but in that case you should have added
the pre-emptive return ahead of that if/else structure, where the existing
"If not IDLE state, just wait ..." test is. My guess though is that we
do need to process error messages in that state, so that the correct
patch looks more like
else
{
+ /*
+ * We're notionally not-IDLE when in pipeline mode we have
+ * completed processing the results of one query and are waiting
+ * for the next one in the pipeline. In this case, as above, just
+ * wait.
+ */
+ if (conn->asyncStatus == PGASYNC_IDLE &&
+ conn->pipelineStatus != PQ_PIPELINE_OFF &&
+ conn->cmd_queue_head != NULL)
+ return;
+
pqInternalNotice(&conn->noticeHooks,
"message type 0x%02x arrived from server while idle",
id);
/* Discard the unexpected message */
conn->inCursor += msgLength;
}
It'd be appropriate to do more than nothing to the comment block above
this if/else chain, too, because really that one ought to explain why we
should consume ERROR when in pipeline state.
(I've not looked at the rest of this patch, just scanned what you did
in fe-protocol3.c, because I wondered if there would be any interaction
with the where-to-advance-inStart changes I'm about to commit. Looks
okay modulo this issue.)
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, alvherre@2ndquadrant.com, pryzby@telsasoft.com, craig.ringer@enterprisedb.com, matthieu.garrigues@gmail.com, zyu@yugabyte.com, ibrar.ahmad@gmail.com
Subject: Re: [HACKERS] PATCH: Batch/pipelining support for libpq
In-Reply-To: <2311038.1615488542@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox