From ec42c5a6537d64071d08fb0f5f06f6321d1129c2 Mon Sep 17 00:00:00 2001 From: David Rowley Date: Thu, 7 Jul 2022 15:11:56 +1200 Subject: [PATCH v1 2/2] Don't add is NOT NULL clause to MinMaxAggInfo transformed queries ... when the column is already non-NULLable. This is not quite there yet. The fact that I had to shuffle preprocess_minmax_aggregates() around makes me think it's wrong. --- src/backend/optimizer/plan/planagg.c | 31 ++++++++++++++++----------- src/backend/optimizer/plan/planmain.c | 8 +++++++ src/backend/optimizer/plan/planner.c | 9 -------- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index 9330908cbf..afb3747acc 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -327,6 +327,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo, Path *sorted_path; Cost path_cost; double path_fraction; + Var *var_target = (Var *) mminfo->target; /* * We are going to construct what is effectively a sub-SELECT query, so @@ -382,18 +383,24 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo, parse->hasDistinctOn = false; parse->hasAggs = false; - /* Build "target IS NOT NULL" expression */ - ntest = makeNode(NullTest); - ntest->nulltesttype = IS_NOT_NULL; - ntest->arg = copyObject(mminfo->target); - /* we checked it wasn't a rowtype in find_minmax_aggs_walker */ - ntest->argisrow = false; - ntest->location = -1; - - /* User might have had that in WHERE already */ - if (!list_member((List *) parse->jointree->quals, ntest)) - parse->jointree->quals = (Node *) - lcons(ntest, (List *) parse->jointree->quals); + /* Check for non-Vars or NULLable Vars and add a NOT NULL tests */ + if (!IsA(var_target, Var) || + !bms_is_member(var_target->varattno - FirstLowInvalidHeapAttributeNumber, + root->simple_rel_array[var_target->varno]->notnullattrs)) + { + /* Build "target IS NOT NULL" expression */ + ntest = makeNode(NullTest); + ntest->nulltesttype = IS_NOT_NULL; + ntest->arg = copyObject(mminfo->target); + /* we checked it wasn't a rowtype in find_minmax_aggs_walker */ + ntest->argisrow = false; + ntest->location = -1; + + /* User might have had that in WHERE already */ + if (!list_member((List *) parse->jointree->quals, ntest)) + parse->jointree->quals = (Node *) + lcons(ntest, (List *) parse->jointree->quals); + } /* Build suitable ORDER BY clause */ sortcl = makeNode(SortGroupClause); diff --git a/src/backend/optimizer/plan/planmain.c b/src/backend/optimizer/plan/planmain.c index c92ddd27ed..f375cac7fd 100644 --- a/src/backend/optimizer/plan/planmain.c +++ b/src/backend/optimizer/plan/planmain.c @@ -165,6 +165,14 @@ query_planner(PlannerInfo *root, */ add_base_rels_to_query(root, (Node *) parse->jointree); + /* + * Preprocess MIN/MAX aggregates, if any. XXX what are the actual + * repercussions of moving this out of grouping_planner() into + * query_planner()? + */ + if (parse->hasAggs) + preprocess_minmax_aggregates(root); + /* * Examine the targetlist and join tree, adding entries to baserel * targetlists for all referenced Vars, and generating PlaceHolderInfo diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 06ad856eac..f6c6c4b8b4 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1425,15 +1425,6 @@ grouping_planner(PlannerInfo *root, double tuple_fraction) parse->hasWindowFuncs = false; } - /* - * Preprocess MIN/MAX aggregates, if any. Note: be careful about - * adding logic between here and the query_planner() call. Anything - * that is needed in MIN/MAX-optimizable cases will have to be - * duplicated in planagg.c. - */ - if (parse->hasAggs) - preprocess_minmax_aggregates(root); - /* * Figure out whether there's a hard limit on the number of rows that * query_planner's result subplan needs to return. Even if we know a -- 2.35.1.windows.2