Re: Asynchronous MergeAppend

Hi!

On Mon, Jul 6, 2026 at 4:42 PM Alexander Pyhalov
<a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
> Alexander Korotkov писал(а) 2026-07-04 02:05:
>
> > While discovering a correctness of callback_pending flag reset in
> > async MergeAppend, I found bug in async plain Append [1]. I've
> > included the fix as 0001 in the current patchset. Other changes to
> > patchset includes.
> >
>
> I expect that this issue can affect both MergeAppend and Append, but
> current test cases don't confirm this.
> If we revert to the old behavior (setting areq->callback_pending to
> false), the tests results of Merge Append tests
> are not changed.

Did you try the test case showed by Gleb [1]. Yet I think it's safe
to follow the fix by Etsuro. In the revised patchset I put this into
ExecAppendBaseAsyncProcessPending() and use for both async append and
async merge append.

> > 0003 contains some cleanups
> > * Unify set_append_references and set_mergeappend_references
> > (setrefs.c)
> > * Merge duplicate Append/MergeAppend cases in explain.c
> > * Merge duplicate Append/MergeAppend cases in
> > planstate_tree_walker_impl (nodeFuncs.c)
>
> This looks good.
>
> > 0006 includes following optimizations and fixes
> > * Fix assertion failure on MergeAppend rescan with an in-flight async
> > request (same as 0001 but for MergeAppend)
>
>
> > * Don't use ExecProcNode() for async subplans. Despite its
> > effectiveness, it doesn't works correctly. When postgres_fdw subplan
> > is executed by ExecProcNode() it interprets the end of async batch as
> > end of the whole data. That effectively leads to skipping the
> > remaining dataset after first batch (100 rows). The test is added.
>
> Ouch. Luckily we've catched this. postgresIterateForeignScan() doesn't
> fetch tuples for async scan states...
>
> > * Make ExecReScanMergeAppend() clear ms_slots. Otherwise subsequent
> > scans can use leftover tuples. The test is also added.
>
> This seems to happen only in updated version of the patch, where
> ExecMergeAppendAsyncGetNext() relies on the fact that
> async requests have been already sent (previously this function firstly
> set slot to NULL prior to sending requests and processing them).

Yes, thank you for clarification. Are you good with this approach?

> > * Document that the needrequest fast-skip is a no-op for MergeAppend
> > (postgres_fdw.c)
> > * Document why create_merge_append_plan discards
> > mark_async_capable_plan's result (createplan.c)
> >
> Looks good.
>
> ExecMergeAppendGetNextSlot() - I'd sligtly prefer to check if mplan is
> member of as_asyncplans and assert that it's a member of
> node->as.valid_asyncplans
> in this case, but I think it doesn't matter much.

OK, I changed to this way.

Links
1. https://www.postgresql.org/message-id/3a3f949c611835620cf675bdb989b1e7%40postgrespro.ru

------
Regards,
Alexander Korotkov
Supabase