(Moved to -hackers)
At Wed, 8 Jun 2022 17:08:47 +0200, Alvaro Herrera <alvhe...@alvh.no-ip.org>
wrote in
> What that Close message is doing is closing the unnamed portal, which
> is otherwise closed implicitly when the next one is opened. That's how
> single-query mode works: if you run a single portal, it'll be kept open.
>
> I believe that the right fix is to not send that Close message in
> PQsendQuery.
Agreed. At least Close message in that context is useless and
PQsendQueryGuts doesn't send it. And removes the Close message surely
fixes the issue.
The doc [1] says:
[1] https://www.postgresql.org/docs/14/protocol-flow.html
> The simple Query message is approximately equivalent to the series
> Parse, Bind, portal Describe, Execute, Close, Sync, using the
> unnamed prepared statement and portal objects and no parameters. One
The current implement of PQsendQueryInternal looks like the result of
a misunderstanding of the doc. In the regression tests, that path is
excercised only for an error case, where no CloseComplete comes.
The attached adds a test for the normal-path of pipelined
PQsendQuery() to simple_pipeline test then modifies that function not
to send Close message. Without the fix, the test fails by "unexpected
notice" even if the trace matches the "expected" content.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From bbc556548cefe1b7a2fa63a33362829a137d91e7 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota....@gmail.com>
Date: Fri, 10 Jun 2022 15:03:52 +0900
Subject: [PATCH] Remove useless Close message from PQsendQuery() in pipleline
mode
PQsendQuery() switch to the extended query protocol while in pipeline
mode. The message sequence mistakenly contained a Close message for
unnamed portal, which may lead to an out-of-sync CloseComplete
message. That Close message is useless and removing it resolves the
issue. A test for this case is added.
Still PQsendQuery() in pipeline-mode prompts ParseComplete and
BindComplete messages that are not seen while non-pipeline mode but
they are in-sync and correctly ignored.
---
src/interfaces/libpq/fe-exec.c | 5 --
.../modules/libpq_pipeline/libpq_pipeline.c | 56 +++++++++++++++++++
.../traces/pipeline_abort.trace | 2 -
.../traces/simple_pipeline.trace | 12 ++++
4 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 919cf5741d..321579319e 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1501,11 +1501,6 @@ PQsendQueryInternal(PGconn *conn, const char *query, bool newQuery)
pqPutInt(0, 4, conn) < 0 ||
pqPutMsgEnd(conn) < 0)
goto sendFailed;
- if (pqPutMsgStart('C', conn) < 0 ||
- pqPutc('P', conn) < 0 ||
- pqPuts("", conn) < 0 ||
- pqPutMsgEnd(conn) < 0)
- goto sendFailed;
entry->queryclass = PGQUERY_EXTENDED;
entry->query = strdup(query);
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 0ff563f59a..0b443a611a 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -968,15 +968,27 @@ test_prepared(PGconn *conn)
fprintf(stderr, "ok\n");
}
+static int n_notice;
+static void
+notice_processor(void *arg, const char *message)
+{
+ n_notice++;
+ fprintf(stderr, "%s", message);
+}
+
static void
test_simple_pipeline(PGconn *conn)
{
PGresult *res = NULL;
const char *dummy_params[1] = {"1"};
Oid dummy_param_oids[1] = {INT4OID};
+ PQnoticeProcessor oldproc;
fprintf(stderr, "simple pipeline... ");
+ n_notice = 0;
+ oldproc = PQsetNoticeProcessor(conn, notice_processor, NULL);
+
/*
* Enter pipeline mode and dispatch a set of operations, which we'll then
* process the results of as they come in.
@@ -1052,6 +1064,50 @@ test_simple_pipeline(PGconn *conn)
if (PQpipelineStatus(conn) != PQ_PIPELINE_OFF)
pg_fatal("Exiting pipeline mode didn't seem to work");
+ if (n_notice > 0)
+ pg_fatal("unexpected notice");
+
+ /* Try the same thing with PQsendQuery */
+ if (PQenterPipelineMode(conn) != 1)
+ pg_fatal("failed to enter pipeline mode: %s", PQerrorMessage(conn));
+
+ if (PQsendQuery(conn, "SELECT 1;") != 1)
+ pg_fatal("failed to send query: %s", PQerrorMessage(conn));
+ PQsendFlushRequest(conn);
+ res = PQgetResult(conn);
+ if (res == NULL)
+ pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
+ PQerrorMessage(conn));
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ pg_fatal("Unexpected result code %s from first pipeline item",
+ PQresStatus(PQresultStatus(res)));
+ PQclear(res);
+
+ res = PQgetResult(conn);
+ if (res != NULL)
+ pg_fatal("expected NULL result");
+
+ if (PQpipelineSync(conn) != 1)
+ pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));
+ res = PQgetResult(conn);
+ if (res == NULL)
+ pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
+ PQerrorMessage(conn));
+ if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
+ pg_fatal("Unexpected result code %s instead of PGRES_PIPELINE_SYNC, error: %s",
+ PQresStatus(PQresultStatus(res)), PQerrorMessage(conn));
+ PQclear(res);
+ res =NULL;
+
+ if (PQexitPipelineMode(conn) != 1)
+ pg_fatal("attempt to exit pipeline mode failed when it should've succeeded: %s",
+ PQerrorMessage(conn));
+
+ if (n_notice > 0)
+ pg_fatal("unexpected notice");
+
+ PQsetNoticeProcessor(conn, oldproc, NULL);
+
fprintf(stderr, "ok\n");
}
diff --git a/src/test/modules/libpq_pipeline/traces/pipeline_abort.trace b/src/test/modules/libpq_pipeline/traces/pipeline_abort.trace
index 3fce548b99..254e485997 100644
--- a/src/test/modules/libpq_pipeline/traces/pipeline_abort.trace
+++ b/src/test/modules/libpq_pipeline/traces/pipeline_abort.trace
@@ -38,7 +38,6 @@ F 26 Parse "" "SELECT 1; SELECT 2" 0
F 12 Bind "" "" 0 0 0
F 6 Describe P ""
F 9 Execute "" 0
-F 6 Close P ""
F 4 Sync
B NN ErrorResponse S "ERROR" V "ERROR" C "42601" M "cannot insert multiple commands into a prepared statement" F "SSSS" L "SSSS" R "SSSS" \x00
B 5 ReadyForQuery I
@@ -46,7 +45,6 @@ F 54 Parse "" "SELECT 1.0/g FROM generate_series(3, -1, -1) g" 0
F 12 Bind "" "" 0 0 0
F 6 Describe P ""
F 9 Execute "" 0
-F 6 Close P ""
F 4 Sync
B 4 ParseComplete
B 4 BindComplete
diff --git a/src/test/modules/libpq_pipeline/traces/simple_pipeline.trace b/src/test/modules/libpq_pipeline/traces/simple_pipeline.trace
index 5c94749bc1..00710bc6bc 100644
--- a/src/test/modules/libpq_pipeline/traces/simple_pipeline.trace
+++ b/src/test/modules/libpq_pipeline/traces/simple_pipeline.trace
@@ -9,4 +9,16 @@ B 33 RowDescription 1 "?column?" NNNN 0 NNNN 4 -1 0
B 11 DataRow 1 1 '1'
B 13 CommandComplete "SELECT 1"
B 5 ReadyForQuery I
+F 17 Parse "" "SELECT 1;" 0
+F 12 Bind "" "" 0 0 0
+F 6 Describe P ""
+F 9 Execute "" 0
+F 4 Flush
+B 4 ParseComplete
+B 4 BindComplete
+B 33 RowDescription 1 "?column?" NNNN 0 NNNN 4 -1 0
+B 11 DataRow 1 1 '1'
+B 13 CommandComplete "SELECT 1"
+F 4 Sync
+B 5 ReadyForQuery I
F 4 Terminate
--
2.31.1
