Horton

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

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread View on postgr.es
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Denis Laxalde <denis.laxalde@dalibo.com>
Cc: Jelte Fennema-Nio <postgres@jeltef.nl>, vignesh C <vignesh21@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Greg Stark <stark@mit.edu>, Tom Lane <tgl@sss.pgh.pa.us>, "Gregory Stark (as CFM)" <stark.cfm@gmail.com>, Jelte Fennema <Jelte.Fennema@microsoft.com>, Daniel Gustafsson <daniel@yesql.se>, Peter Eisentraut <peter@eisentraut.org>, Andres Freund <andres@anarazel.de>, Justin Pryzby <pryzby@telsasoft.com>, Robert Haas <robertmhaas@gmail.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2024-03-06T18:22:46Z
Lists: pgsql-hackers 
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/

Commits

  1. postgres_fdw: re-issue cancel requests a few times if necessary.

  2. Make postgres_fdw's query_cancel test less flaky.

  3. postgres_fdw: Split out the query_cancel test to its own file

  4. Fix copy-paste mistake in PQcancelCreate

  5. Make libpqsrv_cancel's return const char *, not char *

  6. Stabilize postgres_fdw test

  7. libpq-be-fe-helpers.h: wrap new cancel APIs

  8. dblink/isolationtester/fe_utils: Use new cancel API

  9. Put libpq_pipeline cancel test back

  10. Hopefully make libpq_pipeline's new cancel test more reliable

  11. libpq: Add encrypted and non-blocking query cancellation routines

  12. libpq: Move pg_cancel to fe-cancel.c

  13. Add tests for libpq query cancellation APIs

  14. Add missing connection statuses to docs

  15. libpq: Change some static functions to extern

  16. libpq: Add pqReleaseConnHosts function

  17. libpq: Move cancellation related functions to fe-cancel.c

  18. Make spelling of cancelled/cancellation consistent

  19. Be more wary about OpenSSL not setting errno on error.

  20. libpq: Use modern socket flags, if available.

  21. Drop test view when done with it.

  22. Doc: add some doco about using the libpq_pipeline test module.

  23. postgres_fdw: Allow cancellation of transaction control commands.