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

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?