From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Denis Laxalde <denis.laxalde@dalibo.com>
Cc: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Greg Stark <stark@mit.edu>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Gregory Stark (as CFM) <stark.cfm@gmail.com>
Cc: Jelte Fennema <Jelte.Fennema@microsoft.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: Peter Eisentraut <peter@eisentraut.org>
Cc: Andres Freund <andres@anarazel.de>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
Date: Wed, 6 Mar 2024 19:22:46 +0100
Message-ID: <202403061822.spfzqbf7dsgg@alvherre.pgsql> (raw)
In-Reply-To: <2e2933ad-86b4-4fea-8876-5fcc0307e97c@dalibo.com>
Docs: one bogus "that that".
Did we consider having PQcancelConn() instead be called
PQcancelCreate()? I think this better conveys that what we're doing is
create an object that can be used to do something, and that nothing else
is done with it by default. Also, the comment still says
"Asynchronously cancel a query on the given connection. This requires
polling the returned PGcancelConn to actually complete the cancellation
of the query." but this is no longer a good description of what this
function does.
Why do we return a non-NULL pointer from PQcancelConn in the first three
cases where we return errors? (original conn was NULL, original conn is
PGINVALID_SOCKET, pqCopyPGconn returns failure) Wouldn't it make more
sense to free the allocated object and return NULL? Actually, I wonder
if there's any reason at all to return a valid pointer in any failure
cases; I mean, do we really expect that application authors are going to
read/report the error message from a PGcancelConn that failed to be fully
created? Anyway, maybe there are reasons for this; but in any case we
should set ->cancelRequest in all cases, not only after the first tests
for errors.
I think the extra PGconn inside pg_cancel_conn is useless; it would be
simpler to typedef PGcancelConn to PGconn in fe-cancel.c, and remove the
indirection through the extra struct. You're actually dereferencing the
object in two ways in the new code, both by casting the outer object
straight to PGconn (taking advantage that the struct member is first in
the struct), and by using PGcancelConn->conn. This seems pointless. I
mean, if we're going to cast to "PGconn *" in some places anyway, then
we may as well access all members directly. Perhaps, if you want, you
could add asserts that ->cancelRequest is set true in all the
fe-cancel.c functions. Anyway, we'd still have compiler support to tell
you that you're passing the wrong struct to the function. (I didn't
actually try to change the code this way, so I might be wrong.)
We could move the definition of struct pg_cancel to fe-cancel.c. Nobody
outside that needs to know that definition anyway.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"XML!" Exclaimed C++. "What are you doing here? You're not a programming
language."
"Tell that to the people who use me," said XML.
https://burningbird.net/the-parable-of-the-languages/
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: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, denis.laxalde@dalibo.com, postgres@jeltef.nl, vignesh21@gmail.com, thomas.munro@gmail.com, stark@mit.edu, tgl@sss.pgh.pa.us, stark.cfm@gmail.com, Jelte.Fennema@microsoft.com, daniel@yesql.se, peter@eisentraut.org, andres@anarazel.de, pryzby@telsasoft.com, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: [EXTERNAL] Re: Add non-blocking version of PQcancel
In-Reply-To: <202403061822.spfzqbf7dsgg@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox