On Thu, Aug 20, 2020 at 4:36 PM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > This is rebased version.
Thanks for the rebased version! > Come to think of "complex", ExecAsync stuff in this patch might be > too-much for a short-term solution until executor overhaul, if it > comes shortly. (the patch of mine here as a whole is like that, > though..). The queueing stuff in postgres_fdw is, too. Here are some review comments on “ExecAsync stuff” (the 0002 patch): @@ -192,10 +196,20 @@ ExecInitAppend(Append *node, EState *estate, int eflags) i = -1; while ((i = bms_next_member(validsubplans, i)) >= 0) { Plan *initNode = (Plan *) list_nth(node->appendplans, i); + int sub_eflags = eflags; + + /* Let async-capable subplans run asynchronously */ + if (i < node->nasyncplans) + { + sub_eflags |= EXEC_FLAG_ASYNC; + nasyncplans++; + } This would be more ambitious than Thomas’ patch: his patch only allows foreign scan nodes beneath an Append node to be executed asynchronously, but your patch allows any plan nodes beneath it (e.g., local child joins between foreign tables). Right? I think that would be great, but I’m not sure how we execute such plan nodes asynchronously as other parts of your patch seem to assume that only foreign scan nodes beneath an Append are considered as async-capable. Maybe I’m missing something, though. Could you elaborate on that? Your patch (and the original patch by Robert [1]) modified ExecAppend() so that it can process local plan nodes while waiting for the results from remote queries, which would be also a feature that’s not supported by Thomas’ patch, but I’d like to know performance results. Did you do performance testing on that? I couldn’t find that from the archive. +bool +is_async_capable_path(Path *path) +{ + switch (nodeTag(path)) + { + case T_ForeignPath: + { + FdwRoutine *fdwroutine = path->parent->fdwroutine; + + Assert(fdwroutine != NULL); + if (fdwroutine->IsForeignPathAsyncCapable != NULL && + fdwroutine->IsForeignPathAsyncCapable((ForeignPath *) path)) + return true; + } Do we really need to introduce the FDW API IsForeignPathAsyncCapable()? I think we could determine whether a foreign path is async-capable, by checking whether the FDW has the postgresForeignAsyncConfigureWait() API. In relation to the first comment, I noticed this change in the postgres_fdw regression tests: HEAD: EXPLAIN (VERBOSE, COSTS OFF) SELECT a, count(t1) FROM pagg_tab t1 GROUP BY a HAVING avg(b) < 22 ORDER BY 1; QUERY PLAN ------------------------------------------------------------------------ Sort Output: t1.a, (count(((t1.*)::pagg_tab))) Sort Key: t1.a -> Append -> HashAggregate Output: t1.a, count(((t1.*)::pagg_tab)) Group Key: t1.a Filter: (avg(t1.b) < '22'::numeric) -> Foreign Scan on public.fpagg_tab_p1 t1 Output: t1.a, t1.*, t1.b Remote SQL: SELECT a, b, c FROM public.pagg_tab_p1 -> HashAggregate Output: t1_1.a, count(((t1_1.*)::pagg_tab)) Group Key: t1_1.a Filter: (avg(t1_1.b) < '22'::numeric) -> Foreign Scan on public.fpagg_tab_p2 t1_1 Output: t1_1.a, t1_1.*, t1_1.b Remote SQL: SELECT a, b, c FROM public.pagg_tab_p2 -> HashAggregate Output: t1_2.a, count(((t1_2.*)::pagg_tab)) Group Key: t1_2.a Filter: (avg(t1_2.b) < '22'::numeric) -> Foreign Scan on public.fpagg_tab_p3 t1_2 Output: t1_2.a, t1_2.*, t1_2.b Remote SQL: SELECT a, b, c FROM public.pagg_tab_p3 (25 rows) Patched: EXPLAIN (VERBOSE, COSTS OFF) SELECT a, count(t1) FROM pagg_tab t1 GROUP BY a HAVING avg(b) < 22 ORDER BY 1; QUERY PLAN ------------------------------------------------------------------------ Sort Output: t1.a, (count(((t1.*)::pagg_tab))) Sort Key: t1.a -> HashAggregate Output: t1.a, count(((t1.*)::pagg_tab)) Group Key: t1.a Filter: (avg(t1.b) < '22'::numeric) -> Append Async subplans: 3 -> Async Foreign Scan on public.fpagg_tab_p1 t1_1 Output: t1_1.a, t1_1.*, t1_1.b Remote SQL: SELECT a, b, c FROM public.pagg_tab_p1 -> Async Foreign Scan on public.fpagg_tab_p2 t1_2 Output: t1_2.a, t1_2.*, t1_2.b Remote SQL: SELECT a, b, c FROM public.pagg_tab_p2 -> Async Foreign Scan on public.fpagg_tab_p3 t1_3 Output: t1_3.a, t1_3.*, t1_3.b Remote SQL: SELECT a, b, c FROM public.pagg_tab_p3 (18 rows) So, your patch can only handle foreign scan nodes beneath an Append for now? Anyway, I think this would lead to the improved efficiency, considering performance results from Movead [2]. And I think planner changes to make this happen would be a good thing in your patch. That’s all I have for now. Sorry for the delay. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/CA%2BTgmoaXQEt4tZ03FtQhnzeDEMzBck%2BLrni0UWHVVgOTnA6C1w%40mail.gmail.com [2] https://www.postgresql.org/message-id/2020011417113872105895%40highgo.ca