Re: Asynchronous MergeAppend

Etsuro Fujita писал(а) 2026-11-01 20:44:
> Hi Alexander,

Hi.

>
> On Wed, Oct 28, 2026 at 8:37 PM Alexander Pyhalov
> <a(dot)pyhalov(at)postgrespro(dot)ru> wrote:
>> 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
>
> [snip]
>
>> s/csse/case/
>
> Fixed. I also tweaked the comments a bit further.
>

/*
* This is the case when an asynchronous request made by
the
- * requestor for another subplan is currently
in-progress. Skip the
- * configuration even in the case where the requestor is
MergeAppend;
- * in which csse we just defer the configuration until
the preceding
- * request(s) are processed by
ExecMergeAppendAsyncDrain.
+ * requestor for a preceding subplan is currently
in-progress. Skip
+ * the configuration even in the MergeAppend case; in
that case just
+ * difer the configuration until the caller processes
the request(s)
+ * for the preceding subplan(s).
*/

s/difer/defer/

>> 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?
>
> Agreed. I updated fdwhandler.sgml to incorporate that perspective. I
> updated postgres-fdw.sgml and src/backend/executor/README as well.
>

Looks good to me.

> I also adjusted regression test cases and their comments. While doing
> that, I noticed these test cases:
>
> +-- Test async Merge Append rescan when a LIMIT stops the scan before
> every
> +-- async subplan's request has completed, so on the next rescan (for
> the
> +-- next outer row) some subplan's request is still in flight.
> +EXPLAIN (VERBOSE, COSTS OFF)
> +SELECT g.i, s.b FROM generate_series(1, 3) g(i),
> + LATERAL (SELECT b FROM async_pt WHERE a > g.i ORDER BY b LIMIT 1) s;
>
> +-- Test async Merge Append rescan when a LIMIT stops the scan before
> an
> +-- async subplan's already-delivered result was ever consumed (because
> it
> +-- didn't win the race to the heap top). The next rescan (for the
> next
> +-- outer row) must not mistake that leftover result for a fresh one;
> the
> +-- expected result increases each row, so a stale reused value would
> show
> +-- up as the first row's value repeating instead.
> +EXPLAIN (VERBOSE, COSTS OFF)
> +SELECT g.i, s.b FROM generate_series(1, 3) g(i),
> + LATERAL (SELECT b FROM async_pt WHERE a > g.i AND b > g.i * 100
> ORDER BY b LIMIT 1) s;
>
> These are testing the old implementation? There are two more test
> cases for testing the rescan, which seems to me good enough, so I just
> removed them (and adjusted the comments for the remaining test cases).
> Maybe I'm missing something, though.
>

Firstly I've confused these tests with another one:

+-- Test ExecMergeAppendAsyncReset code path that asserts async requests
+-- made for subplans aren't pending anymore at ReScan
+-- SubPlan case
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT
+ ARRAY(SELECT f.i FROM (SELECT b + g.i FROM async_pt WHERE a >
g.i ORDER BY b) f(i) ORDER BY f.i LIMIT 10)
+FROM generate_series(1, 3) g(i);

The reason is obvious - comment is misleading.
In the previous version of the patch it was
-- Test async Merge Append rescan

Now it states

-- Test ExecMergeAppendAsyncReset code path that asserts async requests
-- made for subplans aren't pending anymore at ReScan
-- SubPlan case

The original intention was not this, it was to show that valid async
subplans are recalculated after rescan (and identification of new valid
asyncplans).
Likely, the comment has to be fixed.

And yes, as now we wait for all async requests to deliver results, it
seems we can't have "old" results from previous runs while looking at
node->ms_slots, corresponding
to async plans. So, I suppose that tests that you removed, can be safely
removed.

--
Best regards,
Alexander Pyhalov,
Postgres Professional