On 2022-Jun-10, Kyotaro Horiguchi wrote: > (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.
So, git archaeology led me to this thread https://postgr.es/m/202106072107.d4i55hdscxqj@alvherre.pgsql which is why we added that message in the first place. I was about to push the attached patch (a merged version of Kyotaro's and mine), but now I'm wondering if that's the right approach. Alternatives: - Have the client not complain if it gets CloseComplete in idle state. (After all, it's a pretty useless message, since we already do nothing with it if we get it in BUSY state.) - Have the server not send CloseComplete at all in the cases where the client is not expecting it. Not sure how this would be implemented. - Other ideas? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "That sort of implies that there are Emacs keystrokes which aren't obscure. I've been using it daily for 2 years now and have yet to discover any key sequence which makes any sense." (Paul Thomas)
>From d9f0ff57fe8aa7f963a9411741bb1d68082cc31a Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Wed, 15 Jun 2022 19:56:41 +0200 Subject: [PATCH v2] PQsendQuery: Don't send Close in pipeline mode In commit 4efcf47053ea, we modified PQsendQuery to send a Close message when in pipeline mode. But now we discover that that's not a good thing: under certain circumstances, it causes the server to deliver a CloseComplete message at a time when the client is not expecting it. We failed to noticed it because the tests don't have any scenario where the problem is hit. Remove the offending Close, and add a test case that tickles the problematic scenario. Co-authored-by: Kyotaro Horiguchi <horikyota....@gmail.com> Reported-by: Daniele Varrazzo <daniele.varra...@gmail.com> Discussion: https://postgr.es/m/ca+mi_8bvd0_cw3sumgwpvwdnzxy32itog_16tdyru_1s2gv...@mail.gmail.com --- src/interfaces/libpq/fe-exec.c | 5 -- .../modules/libpq_pipeline/libpq_pipeline.c | 62 +++++++++++++++++++ .../traces/pipeline_abort.trace | 2 - .../traces/simple_pipeline.trace | 12 ++++ 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 4180683194..e2df3a3480 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -1403,11 +1403,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 c27c4e0ada..e24fbfe1cc 100644 --- a/src/test/modules/libpq_pipeline/libpq_pipeline.c +++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c @@ -968,15 +968,29 @@ test_prepared(PGconn *conn) fprintf(stderr, "ok\n"); } +/* Notice processor: print them, and count how many we got */ +static void +notice_processor(void *arg, const char *message) +{ + int *n_notices = (int *) arg; + + (*n_notices)++; + fprintf(stderr, "NOTICE %d: %s", *n_notices, 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; + int n_notices = 0; fprintf(stderr, "simple pipeline... "); + oldproc = PQsetNoticeProcessor(conn, notice_processor, &n_notices); + /* * Enter pipeline mode and dispatch a set of operations, which we'll then * process the results of as they come in. @@ -1052,6 +1066,54 @@ test_simple_pipeline(PGconn *conn) if (PQpipelineStatus(conn) != PQ_PIPELINE_OFF) pg_fatal("Exiting pipeline mode didn't seem to work"); + if (n_notices > 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)); + + /* + * Must not have got any notices here; note bug as described in + * https://postgr.es/m/ca+mi_8bvd0_cw3sumgwpvwdnzxy32itog_16tdyru_1s2gv...@mail.gmail.com + */ + if (n_notices > 0) + pg_fatal("got %d notice(s)", n_notices); + + 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..349034dda6 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 16 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.30.2
