On Tue Nov 18, 2025 at 4:14 AM -03, Alexander Pyhalov wrote:
> Updated the first patch.
>
Thanks for the new version. Some new comments.
v7-0002-MergeAppend-should-support-Async-Foreign-Scan-subpla.patch:
1. Should be "nasyncplans" instead of "nplans"? ExecInitAppend use
"nasyncplans" to allocate the as_asyncresults array.
+ mergestate->ms_asyncresults = (TupleTableSlot **)
+ palloc0(nplans * sizeof(TupleTableSlot *));
+
2. I think that this comment should be updated. IIUC ms_valid_subplans
may not be all subplans because classify_matching_subplans() may move
async ones to ms_valid_asyncplans. Is that right?
/*
* If we've yet to determine the valid subplans then do so now. If
* run-time pruning is disabled then the valid subplans will always be
* set to all subplans.
*/
3. This code comment should also mention the Assert(!bms_is_member(...));?
+ /* The result should be a TupleTableSlot or NULL. */
+ Assert(slot == NULL || IsA(slot, TupleTableSlot));
+ Assert(!bms_is_member(areq->request_index, node->ms_has_asyncresults));
4. It's worth include bms_num_members(node->ms_needrequest) <= 0 check
on ExecMergeAppendAsyncRequest() as an early return? IIUC it would avoid
the bms_is_member(), bms_next_member() and bms_is_empty(needrequest)
calls.
ExecMergeAppendAsyncRequest(MergeAppendState *node, int mplan)
Bitmapset *needrequest;
int i;
+ if (bms_num_members(node->ms_needrequest) <= 0)
+ return false;
+
I'm attaching a diff with some cosmetic changes of indentation and
comments. Feel free to include on the patch or not.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
diff --git a/src/backend/executor/nodeAppend.c
b/src/backend/executor/nodeAppend.c
index c89e2d2787f..a2ed5f71a35 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -1198,6 +1198,9 @@ classify_matching_subplans(AppendState *node)
}
/* No valid async subplans identified. */
- if (!classify_matching_subplans_common(&node->as_valid_subplans,
node->as_asyncplans, &node->as_valid_asyncplans))
+ if (!classify_matching_subplans_common(
+
&node->as_valid_subplans,
+
node->as_asyncplans,
+
&node->as_valid_asyncplans))
node->as_nasyncremain = 0;
}
diff --git a/src/backend/executor/nodeMergeAppend.c
b/src/backend/executor/nodeMergeAppend.c
index 7db41fbf40f..feb25a813b1 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -533,7 +533,10 @@ classify_matching_subplans(MergeAppendState *node)
}
/* No valid async subplans identified. */
- if (!classify_matching_subplans_common(&node->ms_valid_subplans,
node->ms_asyncplans, &node->ms_valid_asyncplans))
+ if (!classify_matching_subplans_common(
+
&node->ms_valid_subplans,
+
node->ms_asyncplans,
+
&node->ms_valid_asyncplans))
node->ms_asyncremain = NULL;
}
@@ -721,11 +724,14 @@ ExecAsyncMergeAppendResponse(AsyncRequest *areq)
{
/* The ending subplan wouldn't have been pending for a
callback. */
Assert(!areq->callback_pending);
- node->ms_asyncremain = bms_del_member(node->ms_asyncremain,
areq->request_index);
+ node->ms_asyncremain = bms_del_member(node->ms_asyncremain,
+
areq->request_index);
return;
}
- node->ms_has_asyncresults = bms_add_member(node->ms_has_asyncresults,
areq->request_index);
+ /* Mark that the async request has a result */
+ node->ms_has_asyncresults = bms_add_member(node->ms_has_asyncresults,
+
areq->request_index);
/* Save result so we can return it. */
node->ms_asyncresults[areq->request_index] = slot;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 2bef54550a3..1dc7e9e6145 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1585,7 +1585,9 @@ GetNeedRequest(PlanState *ps)
/* Common part of classify_matching_subplans() for Append and MergeAppend */
static inline bool
-classify_matching_subplans_common(Bitmapset **valid_subplans, Bitmapset
*asyncplans, Bitmapset **valid_asyncplans)
+classify_matching_subplans_common(Bitmapset **valid_subplans,
+ Bitmapset
*asyncplans,
+ Bitmapset
**valid_asyncplans)
{
Assert(*valid_asyncplans == NULL);
