Optimizer docs typos

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.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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

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

Thanks
Richard

On Mon, May 18, 2020 at 6:00 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().

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

On Fri, May 22, 2020 at 2:53 PM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

Done.

Thanks!

Thanks
Richard