Re: Deleting prepared statements from libpq.

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 <michael@paquier.xyz> 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