Re: Asynchronous MergeAppend

Matheus Alcantara писал(а) 2025-11-12 00:00:
> On Wed Nov 5, 2025 at 3:30 AM -03, Alexander Pyhalov wrote:
>>> So I'm +1 for the idea. I know it's been while since the last patch,
>>> and unfortunully it hasn't received reviews since then. Do you still
>>> plan to work on it? I still need to take a look on the code to see if
>>> I can help with some comments.
>> 
>> I'm still interested in working on this patch, but it didn't get any
>> review (besides internal one). I suppose, Append and MergeAppend nodes
>> need some unification, for example, ExecAppendAsyncEventWait and
>> ExecMergeAppendAsyncEventWait looks the same, both
>> classify_matching_subplans() versions are suspiciously similar. But
>> honestly, patch needs thorough review.
>> 

Hi.
Thanks for review.

> Here are some comments on my first look at the patches. I still don't
> have too much experience with the executor code but I hope that I can
> help with something.
> 
> v5-0001-mark_async_capable-subpath-should-match-subplan.patch
> 
> I don't have to much comments on this, perhaps we could have a commit
> message explaining the reason behind the change.

I've expanded commit message. The issue is that mark_async_capable() 
relies
on the fact that plan node type corresponds to path type. This is not 
true when
(Merge)Append decides to insert Sort node.

> 
> ----
> 
> v5-0002-MergeAppend-should-support-Async-Foreign-Scan-sub.patch
> 
> The AppendState struct has the "as_syncdone", this field is not needed
> on MergeAppendState?

We don't need as_syncdone. Async Append fetches tuple from async subplan 
and waits for them either when they have some data or when there's no 
more sync subplans (as we can return any tuple we receive from 
subplans). But ExecMergeAppend should decide which tuple to return based 
on sort order, so there's no need to remember if we are done with sync 
subplans, as subplans ordering matters, and we can't arbitrary switch 
between them.


> Regarding the duplicated code on classify_matching_subplans I think 
> that
> we can have a more generic function that operates over function
> parameters

I've tried to do so, but there are two issues. There's no suitable 
common header between
nodAppend and nodeMergeAppend. I've put 
classify_matching_subplans_common() into src/include/nodes/execnodes.h
and sure that it's not the best choice. The second issue is with 
as_syncdone, it exists only in AppendState, so
we should check for empty valid_subplans separately. In fact, there's 3 
outcomes for Append 1) no sync plans,
no async plans,  2) no async plans, 3) async plans present, and only 
last two states have meaning
for MergeAppend.

> The GetValidAsyncplans() is not being used

As well as GetValidAsyncRequest(), these parts are used by our FDW, so 
they slipped in the patch. Removed them.

> 
> ----
> We have some reduction of code coverage on nodeMergeAppend.c. The
> significant blocks are on ExecMergeAppendAsyncBegin():
> +    /* If we've yet to determine the valid subplans then do so now. */
> +    if (!node->ms_valid_subplans_identified)
> +    {
> +        node->ms_valid_subplans =
> +            ExecFindMatchingSubPlans(node->ms_prune_state, false, NULL);
> +        node->ms_valid_subplans_identified = true;
> +
> +        classify_matching_subplans(node);
> +    }
> 
> And there are some blocks on ExecReScanMergeAppend(). It's worth adding
> a test case for then? I'm not sure how hard would be to write a
> regression test that cover these blocks.
> 

You are right. There's difference between ExecAppend and 
ExecMergeAppend. Append identifies valid subplans in 
ExecAppendAsyncBegin. MergeAppend - earlier, in ExecMergeAppend(). So 
this is really the dead code. And there was an issue with it, which 
became evident when I've added test for rescan. When we've identified 
new subplans in ExecMergeAppend(), we have to classify them.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional