Horton

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

Jelte Fennema-Nio <postgres@jeltef.nl>

View thread View on postgr.es
From: Jelte Fennema-Nio <postgres@jeltef.nl>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Denis Laxalde <denis.laxalde@dalibo.com>, 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:41:10Z
Lists: pgsql-hackers 
On Wed, 6 Mar 2024 at 19:22, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Docs: one bogus "that that".

will fix

> Did we consider having PQcancelConn() instead be called
> PQcancelCreate()?

Fine by me

> 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.

will fix

> 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?

I think having a useful error message when possible is quite nice. And
I do think people will read/report this error message. Especially
since many people will simply pass it to PQcancelBlocking, whether
it's NULL or not. And then check the status, and then report the error
if the status was CONNECTION_BAD.

> but in any case we
> should set ->cancelRequest in all cases, not only after the first tests
> for errors.

makes sense

> 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.

That sounds nice indeed. I'll try it out.

> We could move the definition of struct pg_cancel to fe-cancel.c.  Nobody
> outside that needs to know that definition anyway.

will do

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.