Horton

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

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Jelte Fennema-Nio <postgres@jeltef.nl>, 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-21T01:30:54Z
Lists: pgsql-hackers

Attachments

Alexander Lakhin <exclusion@gmail.com> writes:
> 17.11.2024 05:33, Tom Lane wrote:
>> Yeah.  This has been happening off-and-on in the buildfarm ever
>> since we added that test.  I'm not sure if it's just "the test
>> is unstable" or if it's telling us there's a problem with the
>> cancel logic.  Scraping the last 3 months worth of buildfarm
>> logs finds these instances:

> Yes, I counted those bf failures at [1] too and posted my explanation
> upthread [2].

Sorry, I'd forgotten about that.  I added some more debug logging
to the modifications you made, and confirmed your theory that the
remote session is ignoring the cancel request because it receives it
while DoingCommandRead is true; which must mean that it hasn't started
the slow query yet.

This implies that the 100ms delay in query_cancel.sql is not reliably
enough for the remote to receive the command, which surprises me,
especially since the failing animals aren't particularly slow ones.
Maybe there is something else happening?  But I do reproduce the
failure after adding your delays, and the patch I'm about to propose
does fix it.

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.  The equivalent thing
to what he suggested would be to not clear the cancel request flag
during DoingCommandRead, instead letting it kill the next query.
But I didn't like the idea for timeouts, and I like it even less for
query cancel.  What I think we should do instead is to re-issue
the cancel request if we've waited a little and nothing came of it.
This corresponds more or less to what a human user would likely do
(or at least this human would).  The attached patch is set up to
re-cancel after 1 second, then 2 more seconds, then 4 more, etc
until we reach the 30-second "it's dead Jim" threshold.

This seems to fix the problem here.  Thoughts?

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.

			regards, tom lane

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.