On Mon Apr 6, 2026 at 9:19 PM -03, Alexander Korotkov wrote:
>> Minor comment on 0005:
>>
>> +static void
>> +ExecMergeAppendAsyncGetNext(MergeAppendState *node, int mplan)
>> +{
>> + /*
>> + * All initial async requests were fired by ExecAppendBaseAsyncBegin.
>>
>> Wondering if we should reference ExecMergeAppendAsyncBegin() instead of
>> ExecAppendBaseAsyncBegin() since this is on nodeMergeAppend, what do you
>> think?
>
> Technically current comment is correct, because async requests are
> essetially fired by ExecAppendBaseAsyncBegin(). But yes, since we're
> on nodeMergeAppend, it would be less confusing to mention
> ExecMergeAppendAsyncBegin(). Fixed.
>
Thanks for the updated version.
I did another round of review and testing and it looks good to me, I did
not find any issue or strange behaviour.
With the refactor the async support for MergeAppend is not too
complicated. My main concern on this patch series is about the refactor
itself, if we are missing something or changing some behaviour without
notice, however as the async/sync Append execution is also using the
refactored code and we have a good code coveraged I fell a bit confident
that we are right here.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
