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(at)paquier(dot)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