pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: postgres_fdw: re-issue cancel requests a few times if necessary. 2+ messages / 1 participants [nested] [flat]
* pgsql: postgres_fdw: re-issue cancel requests a few times if necessary. @ 2024-12-23 20:14 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 2+ messages in thread From: Tom Lane @ 2024-12-23 20:14 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org postgres_fdw: re-issue cancel requests a few times if necessary. Despite the best efforts of commit 0e5c82380, we're still seeing occasional failures of postgres_fdw's query_cancel test in the buildfarm. Investigation suggests that its 100ms timeout is still not enough to reliably ensure that the remote side starts the query before receiving the cancel request --- and if it hasn't, it will just discard the request because it's idle. We discussed allowing a cancel request to kill the next-received query, but that would have wide and perhaps unpleasant side-effects. What seems safer is to make postgres_fdw do what a human user would likely do, which is issue another cancel request if the first one didn't seem to do anything. We'll keep the same overall 30 second grace period before concluding things are broken, but issue additional cancel requests after 1 second, then 2 more seconds, then 4, then 8. (The next one in series is 16 seconds, but we'll hit the 30 second timeout before that.) Having done that, revert the timeout in query_cancel.sql to 10 ms. That will still be enough on most machines, most of the time, for the remote query to start; but now we're intentionally risking the race condition occurring sometimes in the buildfarm, so that the repeat-cancel code path will get some testing. As before, back-patch to v17. We might eventually contemplate back-patching this further, and/or adding similar logic to dblink. But given the lack of field complaints to date, this feels like mostly an exercise in test case stabilization, so v17 is enough. Discussion: https://postgr.es/m/colnv3lzzmc53iu5qoawynr6qq7etn47lmggqr65ddtpjliq5d@glkveb4m6nop Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/c431986de16cc7e7af8ba3af45d846f279606fd3 Modified Files -------------- contrib/postgres_fdw/connection.c | 96 ++++++++++++++++++++------ contrib/postgres_fdw/expected/query_cancel.out | 6 +- contrib/postgres_fdw/sql/query_cancel.sql | 6 +- 3 files changed, 84 insertions(+), 24 deletions(-) ^ permalink raw reply [nested|flat] 2+ messages in thread
* pgsql: postgres_fdw: re-issue cancel requests a few times if necessary. @ 2024-12-23 20:14 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 2+ messages in thread From: Tom Lane @ 2024-12-23 20:14 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org postgres_fdw: re-issue cancel requests a few times if necessary. Despite the best efforts of commit 0e5c82380, we're still seeing occasional failures of postgres_fdw's query_cancel test in the buildfarm. Investigation suggests that its 100ms timeout is still not enough to reliably ensure that the remote side starts the query before receiving the cancel request --- and if it hasn't, it will just discard the request because it's idle. We discussed allowing a cancel request to kill the next-received query, but that would have wide and perhaps unpleasant side-effects. What seems safer is to make postgres_fdw do what a human user would likely do, which is issue another cancel request if the first one didn't seem to do anything. We'll keep the same overall 30 second grace period before concluding things are broken, but issue additional cancel requests after 1 second, then 2 more seconds, then 4, then 8. (The next one in series is 16 seconds, but we'll hit the 30 second timeout before that.) Having done that, revert the timeout in query_cancel.sql to 10 ms. That will still be enough on most machines, most of the time, for the remote query to start; but now we're intentionally risking the race condition occurring sometimes in the buildfarm, so that the repeat-cancel code path will get some testing. As before, back-patch to v17. We might eventually contemplate back-patching this further, and/or adding similar logic to dblink. But given the lack of field complaints to date, this feels like mostly an exercise in test case stabilization, so v17 is enough. Discussion: https://postgr.es/m/colnv3lzzmc53iu5qoawynr6qq7etn47lmggqr65ddtpjliq5d@glkveb4m6nop Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/89962bfef624822786f8fc6268307407fd10bc7f Modified Files -------------- contrib/postgres_fdw/connection.c | 96 ++++++++++++++++++++------ contrib/postgres_fdw/expected/query_cancel.out | 6 +- contrib/postgres_fdw/sql/query_cancel.sql | 6 +- 3 files changed, 84 insertions(+), 24 deletions(-) ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2024-12-23 20:14 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-12-23 20:14 pgsql: postgres_fdw: re-issue cancel requests a few times if necessary. Tom Lane <tgl@sss.pgh.pa.us> 2024-12-23 20:14 pgsql: postgres_fdw: re-issue cancel requests a few times if necessary. Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox