Subject: Re: Deleting prepared statements from libpq.

From: Michael Paquier <[email-masked]>
Date: Fri, 23 Jun 2023 12:59:43 +0900

On Tue, Jun 20, 2023 at 01:42:13PM +0200, Jelte Fennema wrote:

Thanks for updating the patch.

> On Tue, 20 Jun 2023 at 06:18, Michael Paquier <[email-masked]> wrote:
>> The amount of duplication between the describe and close paths
>> concerns me a bit.  Should PQsendClose() and PQsendDescribe() be
>> merged into a single routine (say PQsendCommand), that uses a message
>> type for pqPutMsgStart and a queryclass as extra arguments?
> 
> Done (I called it PQsendTypedCommand)

Okay by me to use this name.

I have a few comments about the tests.  The docs and the code seem to
be in line.

+   if (PQsendClosePrepared(conn, "select_one") != 1)
+       pg_fatal("PQsendClosePortal failed: %s", PQerrorMessage(conn));
+   if (PQpipelineSync(conn) != 1)
+       pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));
+
+   res = PQgetResult(conn);
+   if (res == NULL)
+       pg_fatal("expected non-NULL result");
+   if (PQresultStatus(res) != PGRES_COMMAND_OK)
+       pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
+   PQclear(res);
+   res = PQgetResult(conn);
+   if (res != NULL)
+       pg_fatal("expected NULL result");
+   res = PQgetResult(conn);
+   if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
+       pg_fatal("expected PGRES_PIPELINE_SYNC, got %s", PQresStatus(PQresultStatus(res)));

Okay, so here this checks that a PQresult is returned on the first
call, and that NULL is returned on the second call.  Okay with that.
Perhaps this should add a fprintf(stderr, "closing statement..") at
the top of the test block?  That's a minor point.

+   /*
+    * Also test the blocking close, this should not fail since closing a
+    * non-existent prepared statement is a no-op
+    */
+   res = PQclosePrepared(conn, "select_one");
+   if (PQresultStatus(res) != PGRES_COMMAND_OK)
+       pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
[...]
    res = PQgetResult(conn);
    if (res == NULL)
-       pg_fatal("expected NULL result");
+       pg_fatal("expected non-NULL result");

This should check for the NULL-ness of the result returned for
PQclosePrepared() rather than changing the behavior of the follow-up
calls?

+   if (PQsendClosePortal(conn, "cursor_one") != 1)
+       pg_fatal("PQsendClosePortal failed: %s", PQerrorMessage(conn));
+   if (PQpipelineSync(conn) != 1)
+       pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));
Perhaps I would add an extra fprint about the portal closing test,
just for clarity of the test flow.

@@ -2534,6 +2615,7 @@ sendFailed:
    return 0;
 }
 
+
 /*

Noise diff.
--
Michael

---

From: Jelte Fennema <[email-masked]>
Date: Fri, 23 Jun 2023 09:39:00 +0200

> PQclosePrepared() rather than changing the behavior of the follow-up

To be clear, it didn't actually change the behaviour. I only changed
the error message, since it said the exact opposite of what it was
expecting. I split this minor fix into its own commit now to clarify
that. I think it would even make sense to commit this small patch to
the PG16 branch, since it's a bugfix in the tests (and possibly even
back-patch to others if that's not a lot of work). I changed the error
message to be in line with one from earlier in the test.

I addressed all of your other comments.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 23 Jun 2023 16:53:30 +0900

On Fri, Jun 23, 2023 at 09:39:00AM +0200, Jelte Fennema wrote:
> back-patch to others if that's not a lot of work).

Yes, agreed that this had better be fixed and backpatched.  This
avoids some backpatching fuzz, and that's incorrect as it stands.
--
Michael

---

From: Jelte Fennema <[email-masked]>
Date: Mon, 3 Jul 2023 14:33:55 +0200

@Michael is anything else needed from my side? If not, I'll mark the
commitfest entry as "Ready For Committer".

---

From: Michael Paquier <[email-masked]>
Date: Tue, 4 Jul 2023 08:28:40 +0900

> commitfest entry as "Ready For Committer".

Sure, feel free.  I was planning to look at and play more with it.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 4 Jul 2023 16:09:43 +0900

> Sure, feel free.  I was planning to look at and play more with it.

Well, done.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Wed, 5 Jul 2023 08:39:19 +0900

>> Sure, feel free.  I was planning to look at and play more with it.

For the sake of completeness, as I forgot to send my notes.

+   if (PQsendClosePrepared(conn, "select_one") != 1)
+       pg_fatal("PQsendClosePortal failed: %s", PQerrorMessage(conn));
There was a small copy-pasto here.
--
Michael