From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org,
        Craig Ringer <craig.ringer@enterprisedb.com>,
        Matthieu Garrigues <matthieu.garrigues@gmail.com>,
        Zhihong Yu <zyu@yugabyte.com>, Ibrar Ahmed <ibrar.ahmad@gmail.com>
Subject: Re: [HACKERS] PATCH: Batch/pipelining support for libpq
In-reply-to: <20210311160013.GA13683@alvherre.pgsql>
References: <20210311160013.GA13683@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@2ndquadrant.com>
	message dated "Thu, 11 Mar 2021 13:00:13 -0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2311037.1615488542.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 11 Mar 2021 13:49:02 -0500
Message-ID: <2311038.1615488542@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2311038.1615488542%40sss.pgh.pa.us>
Precedence: bulk

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 hav=
e
+                 * completed processing the results of one query and are =
waiting
+                 * for the next one in the pipeline.  In this case, as ab=
ove, just
+                 * wait.
+                 */
+                if (conn->asyncStatus =3D=3D PGASYNC_IDLE &&
+                    conn->pipelineStatus !=3D PQ_PIPELINE_OFF &&
+                    conn->cmd_queue_head !=3D NULL)
+                    return;
+
                pqInternalNotice(&conn->noticeHooks,
                                 "message type 0x%02x arrived from server =
while idle",
                                 id);
                /* Discard the unexpected message */
                conn->inCursor +=3D 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