Re: Asynchronous Append on postgres_fdw nodes. - Mailing list pgsql-hackers

Thanks for reviewing.

At Sat, 26 Sep 2020 19:45:39 +0900, Etsuro Fujita <etsuro.fujita@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