public inbox for [email protected]
help / color / mirror / Atom feedFrom: Denis Laxalde <[email protected]>
To: Jelte Fennema <[email protected]>
Cc: Greg Stark <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Gregory Stark (as CFM) <[email protected]>
Cc: Jelte Fennema <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Jacob Champion <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
Date: Wed, 29 Mar 2023 10:43:14 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAGECzQQUw3UFP8ivMYg0tt3-vDscSZMRuwpABjkUO2m9=JUSTA@mail.gmail.com>
References: <CAM-w4HNCHEuCBcUxE8H6t15SJKzZd6hKtyHNUTa30VhJxqmuOw@mail.gmail.com>
<CAGECzQSvTjte1-QmzcjnJJkr7BYvuOK4FkjXpXVnRA2AJHuAHw@mail.gmail.com>
<CAGECzQR57o1wdo9fG7grmmabrt1jotHR-T=nQZGXiZRHpvgrGg@mail.gmail.com>
<CAM-w4HOf4+em6TivqZXtbVvpahHRYExxosK5eAyje5C+dSxWSg@mail.gmail.com>
<[email protected]>
<CAM-w4HPyCqVuf=fR-VBB1xcCtUF0+cefpVOv29O-JYbAG4dh2w@mail.gmail.com>
<CAGECzQR7kRY3WKKwx0FdmAA3P2vD+SGALOcXV-dcqMwM4LHV8A@mail.gmail.com>
<CAGECzQTaGtAWmM98q9uup2pjODR8WSOsF_UV3YG7THoqatJApA@mail.gmail.com>
<[email protected]>
<CAGECzQQUw3UFP8ivMYg0tt3-vDscSZMRuwpABjkUO2m9=JUSTA@mail.gmail.com>
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?
view thread (23+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox