Обсуждение: Optimizer docs typos
Attached diff fixes two small typos in the optimizer README. cheers ./daniel
Вложения
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.
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
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 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
On Fri, May 22, 2020 at 2:53 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
Done.
Thanks!
Thanks
Richard