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: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alexander Lakhin <exclusion@gmail.com>, Andres Freund <andres@anarazel.de>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Noah Misch <noah@leadboat.com>, Jacob Champion <jacob.champion@enterprisedb.com>, Denis Laxalde <denis.laxalde@dalibo.com>, vignesh C <vignesh21@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Greg Stark <stark@mit.edu>, "Gregory Stark (as CFM)" <stark.cfm@gmail.com>, Daniel Gustafsson <daniel@yesql.se>, Peter Eisentraut <peter@eisentraut.org>, Justin Pryzby <pryzby@telsasoft.com>, Robert Haas <robertmhaas@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Etsuro Fujita <etsuro.fujita@gmail.com>
Date: 2024-11-21T21:33:06Z
Lists: pgsql-hackers 
On Thu, 21 Nov 2024 at 02:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Anyway, given that info, Jelte's unapplied 0002 patch earlier in the
> thread is not the answer, because this is about dropping a query
> cancel not about losing a timeout interrupt.

Agreed that 0002 does not fix the issue re-reported by Andres (let's
call this issue number 1). But I'm still of the opinion that 0002
fixes a real bug: i.e. a bug which causes timeouts.spec to randomly
fail[1] (let's call this issue number 2).
> This seems to fix the problem here.  Thoughts?

Overall, a good approach to fix issue number 1. I think it would be
best if this was integrated into libpqsrv_cancel instead though. That
way the dblink would benefit from it too.

nit: Maybe call it RETRY_CANCEL_TIME. The RE_ prefix wasn't instantly
obvious what it meant to me, it seemed like an abbreviation when I first saw it.

> BTW, while I didn't do it in the attached, I'm tempted to greatly
> reduce the 100ms delay in query_cancel.sql.  If this does make it
> more robust, we shouldn't need that much time anymore.

Seems sensible to me.


Finally there's a third issue[2] (let's call this issue number 3).
Alexander did some investigation into this issue too[3]. For this one
I have a hard time understanding what is going on, or at least how
this issue only seems to apply to cancel connections. From his
description of the problem and my reading of the code it seems that if
we fail to send the StartupPacket/CancelRequest due to a socket error,
we set the write_failed flag. But we don't actually check this flag
during the CONNECTION_AWAITING_RESPONSE phase of PQconnectPoll, so we
just wait until we reach a timeout because the server never sends us
anything.


[1]: https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures#timeouts.spec_failed_because_of_statement_cancelled_due_to_unexpected_reason
[2]: https://wiki.postgresql.org/wiki/Known_Buildfarm_Test_Failures#dblink.sql_.28and_postgres_fdw.sql.29_fail_on_Windows_due_to_the_cancel_packet_not_sent
[3]: https://www.postgresql.org/message-id/5ea25e4d-1ee2-b9bf-7806-119ffa658826@gmail.com

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.