Thread: havingQual vs hasHavingQual buglets
I came across a couple of places in the planner that are checking for nonempty havingQual; but since these bits run after const-simplification of the HAVING clause, that produces the wrong answer for a constant-true HAVING clause (which'll be folded to empty). Correct code is to check root->hasHavingQual instead. These mistakes only affect cost estimates, and they're sufficiently corner cases that it'd be hard even to devise a reliable test case showing a different plan choice. So I'm not very excited about this, and am thinking of committing only to HEAD. regards, tom lane diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index d98709e5e8..8d7500abfb 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -3357,7 +3357,7 @@ estimate_path_cost_size(PlannerInfo *root, * Get the retrieved_rows and rows estimates. If there are HAVING * quals, account for their selectivity. */ - if (root->parse->havingQual) + if (root->hasHavingQual) { /* Factor in the selectivity of the remotely-checked quals */ retrieved_rows = @@ -3405,7 +3405,7 @@ estimate_path_cost_size(PlannerInfo *root, run_cost += cpu_tuple_cost * numGroups; /* Account for the eval cost of HAVING quals, if any */ - if (root->parse->havingQual) + if (root->hasHavingQual) { QualCost remote_cost; diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 8fc28007f5..4ddaed31a4 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -2575,7 +2575,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel, if (parse->hasAggs || parse->groupClause || parse->groupingSets || - parse->havingQual || + root->hasHavingQual || parse->distinctClause || parse->sortClause || has_multiple_baserels(root))
On Tue, Oct 18, 2022 at 5:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I came across a couple of places in the planner that are checking
for nonempty havingQual; but since these bits run after
const-simplification of the HAVING clause, that produces the wrong
answer for a constant-true HAVING clause (which'll be folded to
empty). Correct code is to check root->hasHavingQual instead.
+1. root->hasHavingQual is set before we do any expression
preprocessing. It should be the right one to check with.
Thanks
Richard
preprocessing. It should be the right one to check with.
Thanks
Richard
On Tue, Oct 18, 2022 at 9:47 AM Richard Guo <guofenglinux@gmail.com> wrote: > On Tue, Oct 18, 2022 at 5:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I came across a couple of places in the planner that are checking >> for nonempty havingQual; but since these bits run after >> const-simplification of the HAVING clause, that produces the wrong >> answer for a constant-true HAVING clause (which'll be folded to >> empty). Correct code is to check root->hasHavingQual instead. The postgres_fdw bits would be my oversight. :-( > +1. root->hasHavingQual is set before we do any expression > preprocessing. It should be the right one to check with. +1 HEAD only seems reasonable. Best regards, Etsuro Fujita
Etsuro Fujita <etsuro.fujita@gmail.com> writes: > On Tue, Oct 18, 2022 at 9:47 AM Richard Guo <guofenglinux@gmail.com> wrote: >> On Tue, Oct 18, 2022 at 5:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I came across a couple of places in the planner that are checking >>> for nonempty havingQual; but since these bits run after >>> const-simplification of the HAVING clause, that produces the wrong >>> answer for a constant-true HAVING clause (which'll be folded to >>> empty). Correct code is to check root->hasHavingQual instead. > The postgres_fdw bits would be my oversight. :-( No worries --- I think the one in set_subquery_pathlist is probably my fault :-( > +1 HEAD only seems reasonable. Pushed that way; thanks for looking. regards, tom lane