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/