Horton

Re: [EXTERNAL] Re: Add non-blocking version of PQcancel

Denis Laxalde <denis.laxalde@dalibo.com>

View thread View on postgr.es
From: Denis Laxalde <denis.laxalde@dalibo.com>
To: Jelte Fennema <postgres@jeltef.nl>
Cc: Greg Stark <stark@mit.edu>, Tom Lane <tgl@sss.pgh.pa.us>, "Gregory Stark (as CFM)" <stark.cfm@gmail.com>, Jelte Fennema <Jelte.Fennema@microsoft.com>, Daniel Gustafsson <daniel@yesql.se>, Peter Eisentraut <peter@eisentraut.org>, Jacob Champion <jchampion@timescale.com>, Andres Freund <andres@anarazel.de>, Justin Pryzby <pryzby@telsasoft.com>, Robert Haas <robertmhaas@gmail.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2023-03-29T08:43:14Z
Lists: pgsql-hackers 
Jelte Fennema a écrit :
> > Namely, I wonder why it returns a PGcancelConn and what's the
> > point of requiring the user to call PQcancelStatus() to see if something
> > got wrong. Maybe it could be defined as:
> >
> >   int PQcancelSend(PGcancelConn *cancelConn);
> >
> > where the return value would be status? And the user would only need to
> > call PQcancelErrorMessage() in case of error. This would leave only one
> > single way to create a PGcancelConn value (i.e. PQcancelConn()), which
> > seems less confusing to me.
> 
> To clarify what you mean, the API would then be like this:
> PGcancelConn cancelConn = PQcancelConn(conn);
> if (PQcancelSend(cancelConn) == CONNECTION_BAD) {
>    printf("ERROR %s\n", PQcancelErrorMessage(cancelConn))
>    exit(1)
> }

I'm not sure it's worth returning the connection status, maybe just an
int value (the return value of connectDBComplete() for instance).

More importantly, not having PQcancelSend() creating the PGcancelConn
makes reuse of that value, passing through PQcancelReset(), more
intuitive. E.g., in the tests:

diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 6764ab513b..91363451af 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -217,17 +217,18 @@ test_cancel(PGconn *conn, const char *conninfo)
 		pg_fatal("failed to run PQrequestCancel: %s", PQerrorMessage(conn));
 	confirm_query_cancelled(conn);
 
+	cancelConn = PQcancelConn(conn);
+
 	/* test PQcancelSend */
 	send_cancellable_query(conn, monitorConn);
-	cancelConn = PQcancelSend(conn);
-	if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
+	if (PQcancelSend(cancelConn) == CONNECTION_BAD)
 		pg_fatal("failed to run PQcancelSend: %s", PQcancelErrorMessage(cancelConn));
 	confirm_query_cancelled(conn);
-	PQcancelFinish(cancelConn);
+
+	PQcancelReset(cancelConn);
 
 	/* test PQcancelConn and then polling with PQcancelPoll */
 	send_cancellable_query(conn, monitorConn);
-	cancelConn = PQcancelConn(conn);
 	if (PQcancelStatus(cancelConn) == CONNECTION_BAD)
 		pg_fatal("bad cancel connection: %s", PQcancelErrorMessage(cancelConn));
 	while (true)

Otherwise, it's not clear if the PGcancelConn created by PQcancelSend()
should be reused or not. But maybe that's a matter of documentation?


> > As part of my testing, I've implemented non-blocking cancellation in
> > Psycopg, based on v16 on this patchset. Overall this worked fine and
> > seems useful; if you want to try it:
> >
> >   https://github.com/dlax/psycopg3/tree/pg16/non-blocking-pqcancel
> 
> That's great to hear! I'll try to take a closer look at that change tomorrow.

See also https://github.com/psycopg/psycopg/issues/534 if you want to
discuss about this.

> > (The only thing I found slightly inconvenient is the need to convey the
> > connection encoding (from PGconn) when handling error message from the
> > PGcancelConn.)
> 
> Could you expand a bit more on this? And if you have any idea on how
> to improve the API with regards to this?

The thing is that we need the connection encoding (client_encoding) when
eventually forwarding the result of PQcancelErrorMessage(), decoded, to
the user. More specifically, it seems to me that we'd the encoding of
the *cancel connection*, but since PQparameterStatus() cannot be used
with a PGcancelConn, I use that of the PGconn. Roughly, in Python:

    encoding = conn.parameter_status(b"client_encoding")
    # i.e, in C: char *encoding PQparameterStatus(conn, "client_encoding");
    cancel_conn = conn.cancel_conn()
    # i.e., in C: PGcancelConn *cancelConn = PQcancelConn(conn);
    # [... then work with with cancel_conn ...]
    if cancel_conn.status == ConnStatus.BAD:
        raise OperationalError(cancel_conn.error_message().decode(encoding))

This feels a bit non-atomic to me; isn't there a risk that
client_encoding be changed between PQparameterStatus(conn) and
PQcancelConn(conn) calls?

So maybe PQcancelParameterStatus(PGcancelConn *cancelConn, char *name)
is needed?

Commits

  1. postgres_fdw: re-issue cancel requests a few times if necessary.

  2. Make postgres_fdw's query_cancel test less flaky.

  3. postgres_fdw: Split out the query_cancel test to its own file

  4. Fix copy-paste mistake in PQcancelCreate

  5. Make libpqsrv_cancel's return const char *, not char *

  6. Stabilize postgres_fdw test

  7. libpq-be-fe-helpers.h: wrap new cancel APIs

  8. dblink/isolationtester/fe_utils: Use new cancel API

  9. Put libpq_pipeline cancel test back

  10. Hopefully make libpq_pipeline's new cancel test more reliable

  11. libpq: Add encrypted and non-blocking query cancellation routines

  12. libpq: Move pg_cancel to fe-cancel.c

  13. Add tests for libpq query cancellation APIs

  14. Add missing connection statuses to docs

  15. libpq: Change some static functions to extern

  16. libpq: Add pqReleaseConnHosts function

  17. libpq: Move cancellation related functions to fe-cancel.c

  18. Make spelling of cancelled/cancellation consistent

  19. Be more wary about OpenSSL not setting errno on error.

  20. libpq: Use modern socket flags, if available.

  21. Drop test view when done with it.

  22. Doc: add some doco about using the libpq_pipeline test module.

  23. postgres_fdw: Allow cancellation of transaction control commands.