Re: add_partial_path() may remove dominated path but still in use

2018年12月31日(月) 22:25 Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>:
>
> On Mon, Dec 31, 2018 at 5:48 PM Kohei KaiGai <kaigai(at)heterodb(dot)com> wrote:
> >
> > 2018年12月31日(月) 13:10 Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>:
> > >
> > > On Sun, Dec 30, 2018 at 9:01 AM Kohei KaiGai <kaigai(at)heterodb(dot)com> wrote:
> > > > 2018年12月30日(日) 4:12 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> > > > >
> > > > > Kohei KaiGai <kaigai(at)heterodb(dot)com> writes:
> > > > > > 2018年12月29日(土) 1:44 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> > > > > >> However, first I'd like to know why this situation is arising in the first
> > > > > >> place. To have the situation you're describing, we'd have to have
> > > > > >> attempted to make some Gather paths before we have all the partial paths
> > > > > >> for the relation they're for. Why is that a good thing to do? It seems
> > > > > >> like such Gathers are necessarily being made with incomplete information,
> > > > > >> and we'd be better off to fix things so that none are made till later.
> > > > >
> > > > > > Because of the hook location, Gather-node shall be constructed with built-in
> > > > > > and foreign partial scan node first, then extension gets a chance to add its
> > > > > > custom paths (partial and full).
> > > > > > At the set_rel_pathlist(), set_rel_pathlist_hook() is invoked next to the
> > > > > > generate_gather_paths().
> > > > >
> > > > > Hmm. I'm inclined to think that we should have a separate hook
> > > > > in which extensions are allowed to add partial paths, and that
> > > > > set_rel_pathlist_hook should only be allowed to add regular paths.
> > >
> > > +1. This idea sounds sensible to me.
> > >
> > > > >
> > > > I have almost same opinion, but the first hook does not need to be
> > > > dedicated for partial paths. As like set_foreign_pathlist() doing, we can
> > > > add both of partial and regular paths here, then generate_gather_paths()
> > > > may generate a Gather-path on top of the best partial-path.
> > > >
> > >
> > > Won't it be confusing for users if we allow both partial and full
> > > paths in first hook and only full paths in the second hook?
> > > Basically, in many cases, the second hook won't be of much use. What
> > > advantage you are seeing in allowing both partial and full paths in
> > > the first hook?
> > >
> > Two advantages. The first one is, it follows same manner of
> > set_foreign_pathlist()
> > which allows to add both of full and partial path if FDW supports parallel-scan.
> > The second one is practical. During the path construction, extension needs to
> > check availability to run (e.g, whether operators in WHERE-clause is supported
> > on GPU device...), calculate its estimated cost and so on. Not a small
> > portion of
> > them are common for both of full and partial path. So, if the first
> > hook accepts to
> > add both kind of paths at once, extension can share the common properties.
> >
>
> You have a point, though I am not sure how much difference it can
> create for cost computation as ideally, both will have different
> costing model. I understand there are some savings by avoiding some
> common work, is there any way to cache the required information?
>
I have no idea for the clean way.
We may be able to have an opaque pointer for extension usage, however,
it may be problematic if multiple extension uses the hook.

> > Probably, the second hook is only used for a few corner case where an extension
> > wants to manipulate path-list already built, like pg_hint_plan.
> >
>
> Okay, but it could be some work for extension authors who are using
> the current hook, not sure they would like to divide the work between
> first and second hook.
>
I guess they don't divide their code, but choose either of them.
In case of PG-Strom, even if there are two hooks around the point, it will use
the first hook only, unless it does not prohibit to call add_path() here.
However, some adjustments are required. Its current implementation makes
GatherPath node with partial CustomScanPath because set_rel_pathlist_hook()
is called after the generate_gather_paths().
Once we could choose the first hook, no need to make a GatherPath by itself,
because PostgreSQL-core will make the path if partial custom-path is enough
reasonable cost. Likely, this adjustment is more preferable one.

Thanks,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai(at)heterodb(dot)com>