Обсуждение: Deleting prepared statements from libpq.

Dmitry Igrishin <dmitigr@gmail.com> writes:
> "there is no libpq function for deleting a prepared statement".
> Could you tell me please, what is the reason for this?

You don't really need one, since you can just use DEALLOCATE.
        regards, tom lane

On Fri, 16 Jun 2023 at 16:26, Craig Ringer <craig@2ndquadrant.com> wrote:
> Nobody's implemented it.
>
> A patch to add PQclosePrepared and PQsendClosePrepared would be welcome. At least, I think so...

This might have been a pretty old thread. But I just took it upon me
to implement these functions (or well I mostly copied the
PQsendDescribe related code and did s/describe/close). I haven't
tested this code yet but I'm pretty sure it should just work (it
compiles at least).

The main reason I'm interested in this is because we're actively
working on implementing named prepared statement support for PgBouncer
in transaction pooling mode. It works with lots of client libraries
already. But sadly it doesn't work with psycopg at the moment, or at
least the closing part does not. And the reason is that psycopg closes
statements using a DEALLOCATE query instead of the Close protocol
message, because libpq does not support sending the Close protocol
message.

Afaict this is not just a problem for PgBouncer its implementation. As
far as I can tell the same is true for the Odyssey connection pooler
(which implemented named prepared statement support first).

On Fri, Jun 16, 2023 at 11:28 PM Jelte Fennema <me@jeltef.nl> wrote:
>
> On Fri, 16 Jun 2023 at 16:26, Craig Ringer <craig@2ndquadrant.com> wrote:
> > Nobody's implemented it.
> >
> > A patch to add PQclosePrepared and PQsendClosePrepared would be welcome. At least, I think so...
>
> This might have been a pretty old thread. But I just took it upon me
> to implement these functions (or well I mostly copied the
> PQsendDescribe related code and did s/describe/close). I haven't
> tested this code yet but I'm pretty sure it should just work (it
> compiles at least).
>
> The main reason I'm interested in this is because we're actively
> working on implementing named prepared statement support for PgBouncer
> in transaction pooling mode. It works with lots of client libraries
> already. But sadly it doesn't work with psycopg at the moment, or at
> least the closing part does not. And the reason is that psycopg closes
> statements using a DEALLOCATE query instead of the Close protocol
> message, because libpq does not support sending the Close protocol
> message.
>
> Afaict this is not just a problem for PgBouncer its implementation. As
> far as I can tell the same is true for the Odyssey connection pooler
> (which implemented named prepared statement support first).


I failed to link it. I don't know why.
if I comment out {assert(PQsendClosePrepared(conn,stmtName) == 1);}
then it works.
I use the following 4 commands, they all yield results, so it seems
PQsendClosePrepare is there.

nm /home/jian/postgres/pg16_test/lib/libpq.so.5.16 | grep PQsendClosePrepare
nm /home/jian/postgres/pg16_test/lib/libpq.so.5 | grep PQsendClosePrepare
 nm /home/jian/postgres/pg16_test/lib/libpq.so | grep PQsendClosePrepare
 nm /home/jian/postgres/pg16_test/lib/libpq.a | grep PQsendClosePrepare

------------------------------------------
/*
gcc -I/home/jian/postgres/pg16_test/include
/home/jian/misc/testlibpq.c -L/home/jian/postgres/pg16_test/lib  -lpq

// nm /home/jian/postgres/pg16_test/lib/libpq.so | grep PQsendClosePrepared
000000000008df90 b __gcov0.PQsendClosePrepared
000000000007fda0 d __gcov_.PQsendClosePrepared
000000000002d060 t PQsendClosePrepared
*/

#include <stdio.h>
#include <stdlib.h>
#include "libpq-fe.h"
#include <assert.h>

void exit_nicely(PGconn *conn)
{
PQfinish(conn);
exit(1);
};

int main()
{
    char    *conninfo = "dbname=test16beta port=5455 host=/tmp user=jian";
PGconn    *conn;
    PGresult   *res;

     /* Make a connection to the database */
conn = PQconnectdb(conninfo);

    /* Check to see that the backend connection was successfully made */
if (PQstatus(conn) != CONNECTION_OK)
    {
fprintf(stderr, "Connection to database failed: %s", PQerrorMessage(conn));
exit_nicely(conn);
}

// PREPARE q3(text, int, float, boolean, smallint) AS
// SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
// ten = $3::bigint OR true = $4 OR odd = $5::int)
// ORDER BY unique1;
// EXECUTE q3('AAAAxx', 5::smallint, 10.5::float, false, 4::bigint);
// select 'text'::regtype::oid,'int'::regtype::oid,'float'::regtype::oid,'bool'::regtype::oid,'smallint'::regtype::oid;

    const char *stmtName = "q3";
    const char *query  =
                    "SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR "
                    "ten = $3::bigint OR true = $4 OR odd = $5::int) "
                    "ORDER BY unique1 ";
    int nParams = 5;
    const Oid oidTypes[5] = {25, 23, 701,16, 21};
    const char *const paramValues[] = {"AAAAxx","5","10.5","false","4"};


    res = PQprepare(conn, stmtName, query,nParams, oidTypes);
    if (PQresultStatus(res) != PGRES_COMMAND_OK)
    {
        printf("here %d\n",__LINE__);
        fprintf(stderr, "PQprepare failed: %s\n",PQresultErrorMessage(res));
        PQclear(res);
    }
    else
    {
        PQclear(res);

        res = PQexecPrepared(conn, stmtName, 5, paramValues,NULL, NULL,0);

        if (PQresultStatus(res) != PGRES_TUPLES_OK)
        {
            PQclear(res);
            printf("PQexecPrepared failed\n");
            exit_nicely(conn);
        }
        assert(PQsendClosePrepared(conn,stmtName) == 1);

        res = PQexec(conn,"select from pg_prepared_statements where
name = 'q3'");
        if (PQresultStatus(res) != PGRES_COMMAND_OK)
        {
            PQclear(res);
            printf("this command should return zero rows now it's not\n");
            exit_nicely(conn);
        }
    }

    PQclear(res);
/* close the connection to the database and cleanup */
    PQfinish(conn);

return 0;
}

On Sat, 17 Jun 2023 at 15:34, jian he <jian.universality@gmail.com> wrote:
> I failed to link it. I don't know why.

Sorry about that. I attached a new patch that allows linking to the
new functions (I forgot to add the functions to exports.txt). This new
patch also adds some basic tests for these new functions.

On Sun, Jun 18, 2023 at 7:04 PM Jelte Fennema <me@jeltef.nl> wrote:
>
> On Sat, 17 Jun 2023 at 15:34, jian he <jian.universality@gmail.com> wrote:
> > I failed to link it. I don't know why.
>
> Sorry about that. I attached a new patch that allows linking to the
> new functions (I forgot to add the functions to exports.txt). This new
> patch also adds some basic tests for these new functions.

previously I cannot link it. with
v2-0001-Support-sending-Close-messages-from-libpq.patch. now I can
compile it, link it, but then run time error.
same c program in the first email.
when I run it ./a.out, then error:
./a.out: symbol lookup error: ./a.out: undefined symbol: PQsendClosePrepared

On Sun, Jun 18, 2023 at 09:23:22PM +0800, jian he wrote:
> previously I cannot link it. with
> v2-0001-Support-sending-Close-messages-from-libpq.patch. now I can
> compile it, link it, but then run time error.
> same c program in the first email.
> when I run it ./a.out, then error:
> ./a.out: symbol lookup error: ./a.out: undefined symbol: PQsendClosePrepared

If you still have problems, it seems to me that one mistake is in not
updating LD_LIBRARY_PATH.  It should point to a version of libpq
compiled with the patch, or -lpq will not be able to resolve correctly
when compiling your test program.
--
Michael

On Sun, Jun 18, 2023 at 01:03:57PM +0200, Jelte Fennema wrote:
> Sorry about that. I attached a new patch that allows linking to the
> new functions (I forgot to add the functions to exports.txt). This new
> patch also adds some basic tests for these new functions.

I am okay with the arguments about pgbouncer and psycopg2.  The
symmetry with the portal description routines makes this proposal easy
to think about.

-   PGQUERY_CLOSE
+   PGQUERY_CLOSE               /* Close Statoment or Portal */

s/Statoment/Statement/.

+ * Available options for close_type are
+ *  'S' to close a prepared statement; or
+ *  'P' to close a portal.
+ * Returns 1 on success and 0 on failure.
+ */
+static int
+PQsendClose(PGconn *conn, char close_type, const char *close_target)

Could it be better for this code path to issue an error if using a
non-supported close_type rather than sending it?  Okay, you are
consistent with desc_type and PQsendDescribe(), just asking if it is
worth doing something about.

+      <listitem>
+       <para>
+        Submits a request to obtain information about the specified
+        prepared statement, and waits for completion.
+<synopsis>
PQclosePrepared() does not request for a description.

+        Submits a request to close the the specified
+        portal, and waits for completion.
s/the the/the/.

+        <xref linkend="libpq-PQclosePortal"/> allows an application to release
+        resources related to a portal previously created portal. If it was a
The end of this sentence looks a bit weird.

Perhaps there should be some tests for the two async routines, as
well?
--
Michael

On Mon, 19 Jun 2023 at 01:57, Michael Paquier <michael@paquier.xyz> wrote:
> +static int
> +PQsendClose(PGconn *conn, char close_type, const char *close_target)
>
> Could it be better for this code path to issue an error if using a
> non-supported close_type rather than sending it?  Okay, you are
> consistent with desc_type and PQsendDescribe(), just asking if it is
> worth doing something about.

Since it's not a publicly exposed function, I think it's fine as is.
Seems easy enough to make sure libpq itself doesn't call it with
unsupported arguments. And even if someone would do that accidentally,
the server would report an error.

> Perhaps there should be some tests for the two async routines, as
> well?

Done

I also fixed all the typos/docs changes you and Jian mentioned, as
well as improving some of the new docs myself.

On Mon, 19 Jun 2023 at 04:52, jian he <jian.universality@gmail.com> wrote:
> > /* Now that it's closed we should get an error when describing */
> > res = PQdescribePortal(conn, "cursor_one");
> > if (PQresultStatus(res) != PGRES_FATAL_ERROR)
> > pg_fatal("expected COMMAND_OK, got %s", PQresStatus(PQresultStatus(res)));
> should it be "if (PQresultStatus(res) == PGRES_FATAL_ERROR)" ?

The check is correct, but the fatal message was incorrect here. We're
intentionally expecting an error here, because that's the easiest way
to see that the portal was closed.

On Mon, 19 Jun 2023 at 11:44, Jelte Fennema <me@jeltef.nl> wrote:
> Done

Now with the actual attachment.

PS. Another connection pooler (PgCat) now also supports prepared
statements, but only using Close not DEALLOCATE:
https://postgresml.org/blog/making-postgres-30-percent-faster-in-production

On Mon, Jun 19, 2023 at 5:50 PM Jelte Fennema <me@jeltef.nl> wrote:
>
> On Mon, 19 Jun 2023 at 11:44, Jelte Fennema <me@jeltef.nl> wrote:
> > Done
>
> Now with the actual attachment.
>
> PS. Another connection pooler (PgCat) now also supports prepared
> statements, but only using Close not DEALLOCATE:
> https://postgresml.org/blog/making-postgres-30-percent-faster-in-production

it works on my local machine.
I am not sure the following two following function comments are right....

/*
 * PQclosePrepared
 *   Obtain information about a previously prepared statement
 * ......

/*
 * PQclosePortal
 *   Obtain information about a previously created portal
 * ....

On Mon, 19 Jun 2023 at 14:17, jian he <jian.universality@gmail.com> wrote:
> I am not sure the following two following function comments are right....

They were incorrect indeed. Attached is a patch with those two updated.

On Mon, 19 Jun 2023 at 14:17, jian he <jian.universality@gmail.com> wrote:
>
> On Mon, Jun 19, 2023 at 5:50 PM Jelte Fennema <me@jeltef.nl> wrote:
> >
> > On Mon, 19 Jun 2023 at 11:44, Jelte Fennema <me@jeltef.nl> wrote:
> > > Done
> >
> > Now with the actual attachment.
> >
> > PS. Another connection pooler (PgCat) now also supports prepared
> > statements, but only using Close not DEALLOCATE:
> > https://postgresml.org/blog/making-postgres-30-percent-faster-in-production
>
> it works on my local machine.
> I am not sure the following two following function comments are right....
>
> /*
>  * PQclosePrepared
>  *   Obtain information about a previously prepared statement
>  * ......
>
> /*
>  * PQclosePortal
>  *   Obtain information about a previously created portal
>  * ....

On Mon, Jun 19, 2023 at 02:49:44PM +0200, Jelte Fennema wrote:
> On Mon, 19 Jun 2023 at 14:17, jian he <jian.universality@gmail.com> wrote:
>> I am not sure the following two following function comments are right....
>
> They were incorrect indeed. Attached is a patch with those two updated.

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?
--
Michael

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)

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

On Fri, 23 Jun 2023 at 05:59, Michael Paquier <michael@paquier.xyz> wrote:
> [...]
>     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?

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.

On Fri, Jun 23, 2023 at 09:39:00AM +0200, Jelte Fennema wrote:
> 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).

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

On Mon, Jul 03, 2023 at 02:33:55PM +0200, Jelte Fennema wrote:
> @Michael is anything else needed from my side? If not, I'll mark the
> commitfest entry as "Ready For Committer".

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

On Tue, Jul 04, 2023 at 08:28:40AM +0900, Michael Paquier wrote:
> Sure, feel free.  I was planning to look at and play more with it.

Well, done.
--
Michael

On Tue, Jul 04, 2023 at 04:09:43PM +0900, Michael Paquier wrote:
> On Tue, Jul 04, 2023 at 08:28:40AM +0900, Michael Paquier wrote:
>> Sure, feel free.  I was planning to look at and play more with it.
>
> Well, done.

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