While working on the big interrupts refactoring, I noticed that
ExecAppendAsyncEventWait() doesn't include WL_LATCH_SET in the wait set.
As a result, it will not wake up on an interrupt like a query cancel
request or sinval catchup.
I'm surprised this has gone unnoticed for so long. It's pretty easy to
reproduce the issue with a foreign table over a view on "SELECT
pg_sleep(100)", for example. Did I miss a bug report?
At first glance the fix is very straightforward; we just need to call
AddWaitEventToSet() to wait for WL_LATCH_SET too. However, when I did
that the straightforward way, the 'postgres_fdw' test got stuck in a
busy-loop in ExecAppendAsyncGetNext().
postgresForeignAsyncConfigureWait() does this:
else if (pendingAreq->requestor != areq->requestor)
{
/*
* This is the case when the in-process request was made by
another
* Append. Note that it might be useless to process the
request made
* by that Append, because the query might not need tuples from
that
* Append anymore; so we avoid processing it to begin a fetch
for the
* given request if possible. If there are any child subplans
of the
* same parent that are ready for new requests, skip the given
* request. Likewise, if there are any configured events other
than
* the postmaster death event, skip it. Otherwise, process the
* in-process request, then begin a fetch to configure the event
* below, because we might otherwise end up with no configured
events
* other than the postmaster death event.
*/
if (!bms_is_empty(requestor->as_needrequest))
return;
if (GetNumRegisteredWaitEvents(set) > 1)
return;
process_pending_request(pendingAreq);
fetch_more_data_begin(areq);
}
When I added the WL_LATCH_SET event to the set before calling this, the
check for "GetNumRegisteredWaitEvents(set) > 1" was always true.
I don't understand what that comment is trying to say. What is an
"in-process request"? And why does postgres_fdw care what other async
subplans are registered in the Append node? If it is legitimate for it
to care, we probably should've abstracted that somehow, rather than
expose that "GetNumRegisteredWaitEvents(set) == 1" means that there are
no other events registered yet.
In any case, the attached patch works and seems like an easy fix for
stable branches at least.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 89b183622d4efbacce8027e8dd29336a7d3182b1 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 3 Mar 2025 17:11:06 +0200
Subject: [PATCH 1/1] Handle interrupts while waiting on Append's async
subplans
We did not wake up on interrupts while waiting on async events on an
async-capable append node. For example, if you tried to cancel the
query, nothing would happen until one of the async subplans becomes
readable. To fix, add WL_LATCH_SET to the WaitEventSet.
---
src/backend/executor/nodeAppend.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index ca0f54d676f..a03a23c18e4 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -326,6 +326,8 @@ ExecAppend(PlanState *pstate)
{
PlanState *subnode;
+ ResetLatch(MyLatch);
+
CHECK_FOR_INTERRUPTS();
/*
@@ -1016,7 +1018,7 @@ ExecAppendAsyncRequest(AppendState *node, TupleTableSlot **result)
static void
ExecAppendAsyncEventWait(AppendState *node)
{
- int nevents = node->as_nasyncplans + 1;
+ int nevents = node->as_nasyncplans + 2;
long timeout = node->as_syncdone ? -1 : 0;
WaitEvent occurred_event[EVENT_BUFFER_SIZE];
int noccurred;
@@ -1041,8 +1043,7 @@ ExecAppendAsyncEventWait(AppendState *node)
}
/*
- * No need for further processing if there are no configured events other
- * than the postmaster death event.
+ * No need for further processing if none of the subplans configured any events.
*/
if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
{
@@ -1051,6 +1052,21 @@ ExecAppendAsyncEventWait(AppendState *node)
return;
}
+ /*
+ * Add the latch to the set, so that we wake up to process the standard
+ * interrupts with CHECK_FOR_INTERRUPTS().
+ *
+ * NOTE: For historical reasons, it's important that this is added to the
+ * WaitEventSet after the ExecAsyncConfigureWait() calls. Namely,
+ * postgres_fdw calls "GetNumRegisteredWaitEvents(set) == 1" to check if
+ * any other events are in the set. That's a bad design, it's
+ * questionable for postgres_fdw to be doing that in the first place, but
+ * we cannot change it now. The pattern has possibly been copied to other
+ * extensions too.
+ */
+ AddWaitEventToSet(node->as_eventset, WL_LATCH_SET, PGINVALID_SOCKET,
+ MyLatch, NULL);
+
/* Return at most EVENT_BUFFER_SIZE events in one call. */
if (nevents > EVENT_BUFFER_SIZE)
nevents = EVENT_BUFFER_SIZE;
--
2.39.5
