Re: Asynchronous MergeAppend

Etsuro Fujita писал(а) 2026-10-27 14:34:
> On Thu, Oct 15, 2026 at 8:34 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
>> I've already started reworking on Alexander
>> Pyhalov's original patch for that. I will post a reworked version as
>> soon as possible.
>
> Here is an updated version of the patch, which I created with AI
> assistance. In the patch I removed the changes made to both the
> MergeAppendState struct and nodeMergeAppend.c so that async execution
> of MergeAppend works (almost) the same as that of Append, and then
> modified them in a more optimized way, which makes the patch pretty
> small/simple.

Hi.

I've looked through the updated patch.
It looks good, but I've found some misprints in comments:

1) postgres_fdw.c - you have a typo here
8708 /*
8709 * Otheewise, process the in-progress request,
begin an asynchronous
8710 * data fetch for the given request, and configure
the file descriptor
8711 * event below.
8712 */
8713 process_pending_request(pendingAreq);
8714 fetch_more_data_begin(areq);

....
8718 /*
8719 * This is the case when an asynchronous request
made by the
8720 * requestor for another subplan is currently
in-progress. Skip the
8721 * configuration even in the case where the
requestor is MergeAppend;
8722 * in which csse we just defer the configuration
until the preceding
8723 * request(s) are processed by
ExecMergeAppendAsyncDrain.
8724 */
8725 return;

s/csse/case/

2) nodeMergeAppend.c:

199 if (mergestate->ms_nasyncplans > 0)
200 {
201 mergestate->ms_asyncrequests = (AsyncRequest **)
202 palloc0(nplans * sizeof(AsyncRequest *));
203

We could use palloc0_array as in surrounding code.

3) One more issue is that we change current semantics of
ForeignAsyncConfigureWait - out-of-tree FDWs
now should handle MergeAppendState in addition to AppendState as
requestor. Should we at least document it?

>
> Some notes about the patch:
>
> * As for the struct, I removed from it ms_asyncresults,
> ms_has_asyncresults, ms_asyncremian, ms_needrequest,
> ms_valid_subplans_identified and ms_valid_asyncplans; and instead
> added to it new member ms_asyncpending, which doesn't exist in
> AppendState. As for the source file, I modified it based on the new
> data struct, reusing some of the code in the original version.
> * I adjusted the change made to create_merge_append_plan(), which
> makes the original 0001 patch useless, so I removed it from the patch.
> * I deduplicated the code in ExecAppendAsyncEventWait() and
> ExecMergeAppendAsyncEventWait() as mentioned upthread. I didn't do so
> for other functions/parts, though, because I don't strongly feel the
> need for that. (I didn't include any of the proposed refactoring,
> either, for the reason I mentioned upthread.)
> * I modified postgresForeignAsyncConfigureWait() a bit further to work
> better with async MergeAppend, and rewrote comments there.
> * I did some editorialization as well, including renaming/relocating
> functions like GetAppendEventSet() in the original version to match
> other functions.
> * I used the regression tests in the v22 patch as-proposed. I think
> some comments should be updated, but I haven't done that yet.
>
> What do you think about that? Maybe I'm missing something, though.
>

--
Best regards,
Alexander Pyhalov,
Postgres Professional