Re: Asynchronous MergeAppend

Alexander Korotkov писал(а) 2026-08-01 00:03:
> 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.

Yes, he found this case when we tested your original fix for
ExecReScanAppend() behavior.
I'm fine with following Etsuro's fix.

>
>> > 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?

Yes, I'm fine with this.
>
>> > * 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.

It seems you've missed the attachment.

--
Best regards,
Alexander Pyhalov,
Postgres Professional