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/