pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17948: libpq seems to misbehave in a pipelining corner case
6+ messages / 4 participants
[nested] [flat]
* BUG #17948: libpq seems to misbehave in a pipelining corner case
@ 2023-05-26 17:27 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 6+ messages in thread
From: PG Bug reporting form @ 2023-05-26 17:27 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: i.trofimow@yandex.ru
The following bug has been logged on the website:
Bug reference: 17948
Logged by: Ivan Trofimov
Email address: i.trofimow@yandex.ru
PostgreSQL version: 15.3
Operating system: Ubuntu 20.04
Description:
As far as i understand, there is an invariant in libpq, that if
PQpipelineSync call finished successfully,
PQresultStatus will eventually return PGRES_PIPELINE_SYNC, or the connection
will be in CONNECTION_BAD state.
This is highlighted in several places in the docs:
1. "PGRES_PIPELINE_SYNC is reported exactly once for each PQpipelineSync at
the corresponding point in the pipeline."
2. "From the client's perspective, after PQresultStatus returns
PGRES_FATAL_ERROR, the pipeline is flagged as aborted.
PQresultStatus will report a PGRES_PIPELINE_ABORTED result for each
remaining queued operation in an aborted pipeline.
The result for PQpipelineSync is reported as PGRES_PIPELINE_SYNC to signal
the end of the aborted pipeline and
resumption of normal result processing. The client must process results with
PQgetResult during error recovery."
So i expect a code like this
if (!PQpipelineSync(conn)) exit(1);
while (PQstatus(conn) != CONNECTION_BAD) {
res = PQgetResult(conn);
if (PQresultStatus(res) == PGRES_PIPELINE_SYNC) {
break;
}
}
to eventually exit the loop.
However, if instead of expected "ReadyToQuery" response server sends an
error and closes the connection (
say, backend terminated by administrator or with a proxy in place and
upstream PG server being dead) this loop
gets stuck in busy-loop.
My understanding is that transitions in pipelining state-machine go like
this:
1. Initial PQgetResult call gets here
https://github.com/postgres/postgres/blob/a817edbf6f302c376f5c0012d19a0474b6bdea88/src/interfaces/li...
2. parseInput gets an error the server send
https://github.com/postgres/postgres/blob/a817edbf6f302c376f5c0012d19a0474b6bdea88/src/interfaces/li...
and switches the state into PGASYNC_READY
3. PQgetResult continues and here
https://github.com/postgres/postgres/blob/a817edbf6f302c376f5c0012d19a0474b6bdea88/src/interfaces/li...
advances the queue, so the Sync entry is gone, and switches into
PGASYNC_PIPELINE_IDLE later.
4. Next PQgetResult call gets here
https://github.com/postgres/postgres/blob/a817edbf6f302c376f5c0012d19a0474b6bdea88/src/interfaces/li...,
and pqPipelineProcessQueue switches into PGASYNC_IDLE here
https://github.com/postgres/postgres/blob/a817edbf6f302c376f5c0012d19a0474b6bdea88/src/interfaces/li...
Now we are stuck in the position where libpq considers the connection being
in CONNECTION_OK state
(because no reads over half-closed socket have been issues),
asyncStatus being PGASYNC_IDLE, pipeline being in PQ_PIPELINE_ABORTED state,
and the expected PGRES_PIPELINE_SYNC never came and never will (because
PGASYNC_IDLE, so PQgetResult returns NULL right away).
I am able to reproduce this behavior reliably via
https://pastebin.com/raw/4j3v3QzC,
linking against libpq5=15.3 and running the program against PostgreSQL
15.2.
Who is at fault here, is it libpq or me misunderstanding/misusing libpq?
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #17948: libpq seems to misbehave in a pipelining corner case
@ 2023-07-16 11:57 Трофимов Иван <i.trofimow@yandex.ru>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 0 replies; 6+ messages in thread
From: Трофимов Иван @ 2023-07-16 11:57 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
<div>Hi.</div><div>Kindly requesting assistance on this, please consider this message as a thread bump.</div>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re:BUG #17948: libpq seems to misbehave in a pipelining corner case
@ 2023-08-05 12:03 Трофимов Иван <i.trofimow@yandex.ru>
0 siblings, 1 reply; 6+ messages in thread
From: Трофимов Иван @ 2023-08-05 12:03 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
<div><div style="background-color:#1e1e1e;color:#d4d4d4;font-family:'droid sans mono' , 'monospace' , monospace;font-size:14px;font-weight:normal;line-height:19px;white-space:pre"><div><span style="color:#ce9178">We've managed to reproduce client <-> server de-synchronization triggered by this, which we are seeing in production.</span></div><div><span style="color:#ce9178">Libpq considers '< 2TDCEZ' a sufficient response to '> BDESS', when according to specification one more 'Z' is expected.</span></div> <div><span style="color:#569cd6">A bit more context and a MRE</span><span style="color:#d4d4d4">: </span><span style="color:#ce9178">https://github.com/itrofimow/libpq_protocol_desync</span></div></div></div;
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #17948: libpq seems to misbehave in a pipelining corner case
@ 2023-12-01 14:20 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Трофимов Иван <i.trofimow@yandex.ru>
0 siblings, 1 reply; 6+ messages in thread
From: Alvaro Herrera @ 2023-12-01 14:20 UTC (permalink / raw)
To: Трофимов Иван <i.trofimow@yandex.ru>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 2023-Aug-05, Трофимов Иван wrote:
> We've managed to reproduce client <-> server de-synchronization triggered
> by this, which we are seeing in production.
> Libpq considers '< 2TDCEZ' a sufficient response to '> BDESS', when
> according to specification one more 'Z' is expected.
>
> A bit more context and a MRE:
> https://github.com/itrofimow/libpq_protocol_desync
Does this patch fix the problems? I think the sync loss is because
we're consuming elements from the command queue even when we shouldn't,
because we don't know for an ERROR whether we should do so or not. What
this patch does is a bit of a hack, I think, but it does solve your test
case (and it doesn't break anything else in the tree) AFAICS. I may
have to play some more with it before gaining confidence, though.
Thanks
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)
Attachments:
[text/x-diff] libpq.patch (3.9K, ../../202312011420.zmxtoxtvu6ip@alvherre.pgsql/2-libpq.patch)
download | inline diff:
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 04610ccf5e..f19edaa5fd 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -2112,19 +2112,24 @@ PQgetResult(PGconn *conn)
break;
case PGASYNC_READY:
+ res = pqPrepareAsyncResult(conn);
/*
- * For any query type other than simple query protocol, we advance
- * the command queue here. This is because for simple query
- * protocol we can get the READY state multiple times before the
- * command is actually complete, since the command string can
- * contain many queries. In simple query protocol, the queue
- * advance is done by fe-protocol3 when it receives ReadyForQuery.
+ * Tell pqCommandQueueAdvance to do its thing, if not in simple
+ * query protocol. XXX rewrite
+ *
+ * When returning a result that is not an error, for any query type
+ * other than simple query protocol, we advance the command queue
+ * here. This is because for simple query protocol we can get the
+ * READY state multiple times before the command is actually
+ * complete, since the command string can contain many queries. In
+ * simple query protocol, the queue advance is done by fe-protocol3
+ * when it receives ReadyForQuery.
*/
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE)
- pqCommandQueueAdvance(conn);
- res = pqPrepareAsyncResult(conn);
+ pqCommandQueueAdvance(conn, res->resultStatus == PGRES_FATAL_ERROR);
+
if (conn->pipelineStatus != PQ_PIPELINE_OFF)
{
/*
@@ -3090,15 +3095,28 @@ PQexitPipelineMode(PGconn *conn)
* pqCommandQueueAdvance
* Remove one query from the command queue, when we receive
* all results from the server that pertain to it.
+ *
+ * Note: when the result we're processing is an error, we must only advance
+ * the queue for message types other than SYNC. This is because we might
+ * get other errors arbitrarily (say, connection closed), but we must not go
+ * past the synchronization point until the server agrees that that's where we
+ * are.
*/
void
-pqCommandQueueAdvance(PGconn *conn)
+pqCommandQueueAdvance(PGconn *conn, bool isError)
{
PGcmdQueueEntry *prevquery;
if (conn->cmd_queue_head == NULL)
return;
+ /*
+ * In case of error, we only allow the queue to advance if the entry
+ * to be removed is not SYNC.
+ */
+ if (isError && conn->cmd_queue_head->queryclass == PGQUERY_SYNC)
+ return;
+
/* delink from queue */
prevquery = conn->cmd_queue_head;
conn->cmd_queue_head = conn->cmd_queue_head->next;
@@ -3162,7 +3180,8 @@ pqPipelineProcessQueue(PGconn *conn)
*/
if (conn->cmd_queue_head == NULL)
{
- conn->asyncStatus = PGASYNC_IDLE;
+ if (conn->pipelineStatus != PQ_PIPELINE_ABORTED)
+ conn->asyncStatus = PGASYNC_IDLE;
return;
}
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 5613c56b14..017999057d 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -246,7 +246,7 @@ pqParseInput3(PGconn *conn)
*/
if (conn->cmd_queue_head &&
conn->cmd_queue_head->queryclass == PGQUERY_SIMPLE)
- pqCommandQueueAdvance(conn);
+ pqCommandQueueAdvance(conn, false);
conn->asyncStatus = PGASYNC_IDLE;
}
break;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 22bc682ffc..8518ac6d76 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -698,7 +698,7 @@ extern void pqSaveMessageField(PGresult *res, char code,
extern void pqSaveParameterStatus(PGconn *conn, const char *name,
const char *value);
extern int pqRowProcessor(PGconn *conn, const char **errmsgp);
-extern void pqCommandQueueAdvance(PGconn *conn);
+extern void pqCommandQueueAdvance(PGconn *conn, bool isError);
extern int PQsendQueryContinue(PGconn *conn, const char *query);
/* === in fe-protocol3.c === */
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #17948: libpq seems to misbehave in a pipelining corner case
@ 2023-12-02 20:56 Ivan Trofimov <i.trofimow@yandex.ru>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 6+ messages in thread
From: Ivan Trofimov @ 2023-12-02 20:56 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Thank you for looking into this, much appreciated.
Yes, I can confirm that the patch fixes both the busy-loop and
client<->server desynchronization problem for me.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: BUG #17948: libpq seems to misbehave in a pipelining corner case
@ 2023-12-04 18:47 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Ivan Trofimov <i.trofimow@yandex.ru>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2023-12-04 18:47 UTC (permalink / raw)
To: Ivan Trofimov <i.trofimow@yandex.ru>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
On 2023-Dec-02, Ivan Trofimov wrote:
> Thank you for looking into this, much appreciated.
>
> Yes, I can confirm that the patch fixes both the busy-loop and
> client<->server desynchronization problem for me.
Great, thanks for the confirmation.
Mulling it over while writing better comments for it, I realized that it
could still be somewhat brittle, and it's also confusing that part of
the logic is in PQgetResult and the rest in pqCommandQueueAdvance; so I
moved it all to the latter, which also lets me document it more clearly
in the comments. I addressed the brittleness by changing what to test:
instead of just "it's an error, then do nothing if the queue has a
SYNC", we can verify that we actually have a matching SYNC in both
places. This seems much better.
Patch v2 attached. I'll be pushing this to 14+ tomorrow, unless
something ugly comes up.
Thanks for reporting this problem!
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)
Attachments:
[text/x-diff] v2-0001-Fix-handling-of-errors-in-libpq-pipelines.patch (0B, ../../202312041847.iur3zphjejur@alvherre.pgsql/2-v2-0001-Fix-handling-of-errors-in-libpq-pipelines.patch)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-12-04 18:47 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 17:27 BUG #17948: libpq seems to misbehave in a pipelining corner case PG Bug reporting form <noreply@postgresql.org>
2023-07-16 11:57 ` Трофимов Иван <i.trofimow@yandex.ru>
2023-08-05 12:03 Re:BUG #17948: libpq seems to misbehave in a pipelining corner case Трофимов Иван <i.trofimow@yandex.ru>
2023-12-01 14:20 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-12-02 20:56 ` Ivan Trofimov <i.trofimow@yandex.ru>
2023-12-04 18:47 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
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