Обсуждение: Optimizer docs typos

Attached diff fixes two small typos in the optimizer README.

cheers ./daniel

On Mon, May 18, 2020 at 6:56 PM Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, May 18, 2020 at 11:31 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>> Attached diff fixes two small typos in the optimizer README.

> Pushed, thanks.

Thank you!

Best regards,
Etsuro Fujita

On Mon, May 18, 2020 at 7:45 PM Richard Guo <guofenglinux@gmail.com> wrote:
> In this same README doc, another suspicious typo to me, which happens in
> section "Optimizer Functions", is in the prefix to query_planner(),
> we should have three dashes, rather than two, since query_planner() is
> called within grouping_planner().
>
> diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README
> index 7dcab9a..bace081 100644
> --- a/src/backend/optimizer/README
> +++ b/src/backend/optimizer/README
> @@ -315,7 +315,7 @@ set up for recursive handling of subqueries
>    preprocess target list for non-SELECT queries
>    handle UNION/INTERSECT/EXCEPT, GROUP BY, HAVING, aggregates,
>         ORDER BY, DISTINCT, LIMIT
> ---query_planner()
> +---query_planner()
>     make list of base relations used in query
>     split up the qual into restrictions (a=1) and joins (b=c)
>     find qual clauses that enable merge and hash joins

Yeah, you are right.  Another one would be in the prefix to
standard_join_search(); I think it might be better to have six dashes,
rather than five, because standard_join_search() is called within
make_rel_from_joinlist().

Best regards,
Etsuro Fujita

On Tue, May 19, 2020 at 7:35 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Mon, May 18, 2020 at 7:45 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > In this same README doc, another suspicious typo to me, which happens in
> > section "Optimizer Functions", is in the prefix to query_planner(),
> > we should have three dashes, rather than two, since query_planner() is
> > called within grouping_planner().
> >
> > diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README
> > index 7dcab9a..bace081 100644
> > --- a/src/backend/optimizer/README
> > +++ b/src/backend/optimizer/README
> > @@ -315,7 +315,7 @@ set up for recursive handling of subqueries
> >    preprocess target list for non-SELECT queries
> >    handle UNION/INTERSECT/EXCEPT, GROUP BY, HAVING, aggregates,
> >         ORDER BY, DISTINCT, LIMIT
> > ---query_planner()
> > +---query_planner()
> >     make list of base relations used in query
> >     split up the qual into restrictions (a=1) and joins (b=c)
> >     find qual clauses that enable merge and hash joins
>
> Yeah, you are right.  Another one would be in the prefix to
> standard_join_search(); I think it might be better to have six dashes,
> rather than five, because standard_join_search() is called within
> make_rel_from_joinlist().

Here is a patch including the change I proposed.  (Yet another thing I
noticed is the indent spaces for join_search_one_level(): that
function is called within standard_join_search(), so it would be
better to have one extra space, for consistency with others (eg,
set_base_rel_pathlists() called from make_one_rel()), but that would
be too nitpicking.)  This is more like an improvement, so I'll apply
the patch to HEAD only, if no objestions.

Best regards,
Etsuro Fujita

At Wed, 20 May 2020 19:17:48 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in 
> On Tue, May 19, 2020 at 7:35 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Mon, May 18, 2020 at 7:45 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > > ---query_planner()
> > > +---query_planner()
> > >     make list of base relations used in query
> > >     split up the qual into restrictions (a=1) and joins (b=c)
> > >     find qual clauses that enable merge and hash joins
> >
> > Yeah, you are right.  Another one would be in the prefix to
> > standard_join_search(); I think it might be better to have six dashes,
> > rather than five, because standard_join_search() is called within
> > make_rel_from_joinlist().
> 
> Here is a patch including the change I proposed.  (Yet another thing I
> noticed is the indent spaces for join_search_one_level(): that
> function is called within standard_join_search(), so it would be
> better to have one extra space, for consistency with others (eg,
> set_base_rel_pathlists() called from make_one_rel()), but that would
> be too nitpicking.)  This is more like an improvement, so I'll apply
> the patch to HEAD only, if no objestions.

The original proposal for query_planner looks fine.

The description for make_rel_from_joinlist() and that for
standard_join_search() are at the same indentation depth.  And it is
also strange that seemingly there is no line for level-5
indentation. If we make standard_join_search() a 6th-hyphened level
item, indentation of the surrounding descriptions needs a fix.

----make_one_rel()
     set_base_rel_pathlists()
      find seqscan and all index paths for each base relation
      find selectivity of columns used in joins
     make_rel_from_joinlist()
      hand off join subproblems to a plugin, GEQO, or standard_join_search()
------standard_join_search()
      call join_search_one_level() for each level of join tree needed
      join_search_one_level():
        For each joinrel of the prior level, do make_rels_by_clause_joins()
        if it has join clauses, or make_rels_by_clauseless_joins() if not.

Looking the description for make_rel_from_joinlist(), it seems to me
that the author is thinking that make_rel_from_joinlist() is mere the
distributor for join subproblems and isn't worth an indent level.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, May 21, 2020 at 11:25 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> At Wed, 20 May 2020 19:17:48 +0900, Etsuro Fujita <etsuro.fujita@gmail.com> wrote in
> > Here is a patch including the change I proposed.  (Yet another thing I
> > noticed is the indent spaces for join_search_one_level(): that
> > function is called within standard_join_search(), so it would be
> > better to have one extra space, for consistency with others (eg,
> > set_base_rel_pathlists() called from make_one_rel()), but that would
> > be too nitpicking.)  This is more like an improvement, so I'll apply
> > the patch to HEAD only, if no objestions.

> The description for make_rel_from_joinlist() and that for
> standard_join_search() are at the same indentation depth.  And it is
> also strange that seemingly there is no line for level-5
> indentation. If we make standard_join_search() a 6th-hyphened level
> item, indentation of the surrounding descriptions needs a fix.
>
> ----make_one_rel()
>      set_base_rel_pathlists()
>       find seqscan and all index paths for each base relation
>       find selectivity of columns used in joins
>      make_rel_from_joinlist()
>       hand off join subproblems to a plugin, GEQO, or standard_join_search()
> ------standard_join_search()
>       call join_search_one_level() for each level of join tree needed
>       join_search_one_level():
>         For each joinrel of the prior level, do make_rels_by_clause_joins()
>         if it has join clauses, or make_rels_by_clauseless_joins() if not.

I don't think it's odd that we won't have 5-dashes indentation,
because I think we have 5-spaces indentation for
set_base_rel_pathlists() and make_rel_from_joinlist().  (I think the
dash indentation of optimizer functions such as standard_join_search()
just means that the dash-indented functions are more important
compared to other space-indented functions IMO.)  My point is that we
should adjust the dash or space indentation so that a deeper level of
indentation indicates that the outer optimizer function calls the
inner optimizer function.

Thanks!

Best regards,
Etsuro Fujita

On Wed, May 20, 2020 at 7:17 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Tue, May 19, 2020 at 7:35 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Mon, May 18, 2020 at 7:45 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > > In this same README doc, another suspicious typo to me, which happens in
> > > section "Optimizer Functions", is in the prefix to query_planner(),
> > > we should have three dashes, rather than two, since query_planner() is
> > > called within grouping_planner().
> > >
> > > diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README
> > > index 7dcab9a..bace081 100644
> > > --- a/src/backend/optimizer/README
> > > +++ b/src/backend/optimizer/README
> > > @@ -315,7 +315,7 @@ set up for recursive handling of subqueries
> > >    preprocess target list for non-SELECT queries
> > >    handle UNION/INTERSECT/EXCEPT, GROUP BY, HAVING, aggregates,
> > >         ORDER BY, DISTINCT, LIMIT
> > > ---query_planner()
> > > +---query_planner()
> > >     make list of base relations used in query
> > >     split up the qual into restrictions (a=1) and joins (b=c)
> > >     find qual clauses that enable merge and hash joins
> >
> > Yeah, you are right.  Another one would be in the prefix to
> > standard_join_search(); I think it might be better to have six dashes,
> > rather than five, because standard_join_search() is called within
> > make_rel_from_joinlist().
>
> Here is a patch including the change I proposed.  (Yet another thing I
> noticed is the indent spaces for join_search_one_level(): that
> function is called within standard_join_search(), so it would be
> better to have one extra space, for consistency with others (eg,
> set_base_rel_pathlists() called from make_one_rel()), but that would
> be too nitpicking.)  This is more like an improvement, so I'll apply
> the patch to HEAD only, if no objestions.

Done.

Best regards,
Etsuro Fujita