Alexander Korotkov писал(а) 2026-08-01 00:03:
Hi!
On Mon, Jul 6, 2026 at 4:42 PM Alexander Pyhalov
<[email protected]> 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
