Thanks for reviewing.
At Sat, 26 Sep 2020 19:45:39 +0900, Etsuro Fujita <etsuro.fuj...@gmail.com>
wrote in
> > 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
Right. It is intended to work any place, but all upper nodes up to the
common node must be "async-aware and capable" for the machinery to work. So it
doesn't work currently since Append is the only async-aware node.
> 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?
Right about this patch. As a trial at hand, in my faint memory, some
join methods and some aggregaioion can be async-aware but they are not
included in this patch not to bloat it with more complex stuff.
> 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.
At least, even though theoretically, I think it's obvious that it's
performant to do something than just sitting waitng for the next tuple
to come from abroad. (I's not so obvious for slow local
vs. hyperspeed-remotes configuration, but...)
> +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.
Note that the API routine takes a path, but it's just that a child
path in a certain form theoretically can obstruct async behavior.
> 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
Yes, as I wrote above. Append-Foreign is the most promising and
suitable as an example. (and... Agg/WindowAgg are the hardest nodes
to make async-aware.)
> 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.
Thanks.
> That’s all I have for now. Sorry for the delay.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
