Re: libpq async duplicate error results - Mailing list pgsql-hackers

From Tom Lane
Subject Re: libpq async duplicate error results
Date
Msg-id 2942076.1651852054@sss.pgh.pa.us
Whole thread Raw
In response to Re: libpq async duplicate error results  (Tom Lane <tgl@sss.pgh.pa.us>)
List pgsql-hackers
I wrote:
> What is happening is that this bit in PQsendQueryStart is deciding
> not to clear the message buffer because conn->cmd_queue_head isn't
> NULL:

>     /*
>      * If this is the beginning of a query cycle, reset the error state.
>      * However, in pipeline mode with something already queued, the error
>      * buffer belongs to that command and we shouldn't clear it.
>      */
>     if (newQuery && conn->cmd_queue_head == NULL)
>         pqClearConnErrorState(conn);

> So apparently the fault is with the pipelined-query logic.  I'm
> not sure what cleanup step is missing though.

I experimented with the attached patch, which is based on the idea
that clearing the command queue is being done in entirely the wrong
place.  It's more appropriate to do it where we drop unsent output
data, ie in pqDropConnection not pqDropServerData.  The fact that
it was jammed into the latter without any commentary doesn't do much
to convince me that that placement was thought about carefully.

This fixes the complained-of symptom and still passes check-world.
However, I wonder whether cutting the queue down to completely empty
is the right thing.  Why is the queue set up like this in the first
place --- that is, why don't we remove an entry the moment the
command is sent?  I also wonder whether pipelineStatus ought to
get reset here.  We aren't resetting asyncStatus here, so maybe it's
fine, but I'm not quite sure.

            regards, tom lane

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 4c12f1411f..6e936bbff3 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -377,6 +377,7 @@ static int    connectDBStart(PGconn *conn);
 static int    connectDBComplete(PGconn *conn);
 static PGPing internal_ping(PGconn *conn);
 static PGconn *makeEmptyPGconn(void);
+static void pqFreeCommandQueue(PGcmdQueueEntry *queue);
 static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
 static void freePGconn(PGconn *conn);
 static void closePGconn(PGconn *conn);
@@ -462,6 +463,12 @@ pqDropConnection(PGconn *conn, bool flushInput)
     /* Always discard any unsent data */
     conn->outCount = 0;

+    /* Likewise, discard any pending pipelined commands */
+    pqFreeCommandQueue(conn->cmd_queue_head);
+    conn->cmd_queue_head = conn->cmd_queue_tail = NULL;
+    pqFreeCommandQueue(conn->cmd_queue_recycle);
+    conn->cmd_queue_recycle = NULL;
+
     /* Free authentication/encryption state */
 #ifdef ENABLE_GSS
     {
@@ -569,12 +576,6 @@ pqDropServerData(PGconn *conn)
     }
     conn->notifyHead = conn->notifyTail = NULL;

-    pqFreeCommandQueue(conn->cmd_queue_head);
-    conn->cmd_queue_head = conn->cmd_queue_tail = NULL;
-
-    pqFreeCommandQueue(conn->cmd_queue_recycle);
-    conn->cmd_queue_recycle = NULL;
-
     /* Reset ParameterStatus data, as well as variables deduced from it */
     pstatus = conn->pstatus;
     while (pstatus != NULL)

pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: make MaxBackends available in _PG_init
Next
From: Nathan Bossart
Date:
Subject: Re: Possible corruption by CreateRestartPoint at promotion