public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matheus Alcantara <[email protected]>
To: Alexander Pyhalov <[email protected]>
To: Matheus Alcantara <[email protected]>
Cc: Alena Rybakina <[email protected]>
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Asynchronous MergeAppend
Date: Wed, 19 Nov 2025 18:51:35 -0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAFY6G8d3Yvxa_kRQA24BsJhwqfmSCv1ujiv_7b6g5isf-ZTs_Q@mail.gmail.com>
<[email protected]>
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);
Attachments:
[text/plain] diff.txt (2.6K, 2-diff.txt)
download | inline diff:
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);
view thread (33+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Asynchronous MergeAppend
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox