diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 9d912a8445..e165810e80 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -2088,22 +2088,14 @@ fix_scan_expr_mutator(Node *node, fix_scan_expr_context *context) if (IsA(node, Aggref)) { Aggref *aggref = (Aggref *) node; + Param *aggparam; /* See if the Aggref should be replaced by a Param */ - if (context->root->minmax_aggs != NIL && - list_length(aggref->args) == 1) + aggparam = find_minmax_agg_replacement_param(context->root, aggref); + if (aggparam != NULL) { - TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args); - ListCell *lc; - - foreach(lc, context->root->minmax_aggs) - { - MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(lc); - - if (mminfo->aggfnoid == aggref->aggfnoid && - equal(mminfo->target, curTarget->expr)) - return (Node *) copyObject(mminfo->param); - } + /* Make a copy of the Param for paranoia's sake */ + return (Node *) copyObject(aggparam); } /* If no match, just fall through to process it normally */ } @@ -3030,22 +3022,14 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context) if (IsA(node, Aggref)) { Aggref *aggref = (Aggref *) node; + Param *aggparam; /* See if the Aggref should be replaced by a Param */ - if (context->root->minmax_aggs != NIL && - list_length(aggref->args) == 1) + aggparam = find_minmax_agg_replacement_param(context->root, aggref); + if (aggparam != NULL) { - TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args); - ListCell *lc; - - foreach(lc, context->root->minmax_aggs) - { - MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(lc); - - if (mminfo->aggfnoid == aggref->aggfnoid && - equal(mminfo->target, curTarget->expr)) - return (Node *) copyObject(mminfo->param); - } + /* Make a copy of the Param for paranoia's sake */ + return (Node *) copyObject(aggparam); } /* If no match, just fall through to process it normally */ } @@ -3199,6 +3183,38 @@ set_windowagg_runcondition_references(PlannerInfo *root, return newlist; } +/* + * find_minmax_agg_replacement_param + * If the given Aggref is one that we are optimizing into a subquery + * (cf. planagg.c), then return the Param that should replace it. + * Else return NULL. + * + * This is exported so that SS_finalize_plan can use it before setrefs.c runs. + * Note that it will not find anything until we have built a Plan from a + * MinMaxAggPath, as root->minmax_aggs will never be filled otherwise. + */ +Param * +find_minmax_agg_replacement_param(PlannerInfo *root, Aggref *aggref) +{ + if (root->minmax_aggs != NIL && + list_length(aggref->args) == 1) + { + TargetEntry *curTarget = (TargetEntry *) linitial(aggref->args); + ListCell *lc; + + foreach(lc, root->minmax_aggs) + { + MinMaxAggInfo *mminfo = (MinMaxAggInfo *) lfirst(lc); + + if (mminfo->aggfnoid == aggref->aggfnoid && + equal(mminfo->target, curTarget->expr)) + return mminfo->param; + } + } + return NULL; +} + + /***************************************************************************** * QUERY DEPENDENCY MANAGEMENT *****************************************************************************/ diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index a1957883ba..61b1238431 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -2849,8 +2849,8 @@ finalize_plan(PlannerInfo *root, Plan *plan, } /* - * finalize_primnode: add IDs of all PARAM_EXEC params appearing in the given - * expression tree to the result set. + * finalize_primnode: add IDs of all PARAM_EXEC params that appear (or will + * appear) in the given expression tree to the result set. */ static bool finalize_primnode(Node *node, finalize_primnode_context *context) @@ -2867,7 +2867,26 @@ finalize_primnode(Node *node, finalize_primnode_context *context) } return false; /* no more to do here */ } - if (IsA(node, SubPlan)) + else if (IsA(node, Aggref)) + { + /* + * Check to see if the aggregate will be replaced by a Param + * referencing a subquery output during setrefs.c. If so, we must + * account for that Param here. (For various reasons, it's not + * convenient to perform that substitution earlier than setrefs.c, nor + * to perform this processing after setrefs.c. Thus we need a wart + * here.) + */ + Aggref *aggref = (Aggref *) node; + Param *aggparam; + + aggparam = find_minmax_agg_replacement_param(context->root, aggref); + if (aggparam != NULL) + context->paramids = bms_add_member(context->paramids, + aggparam->paramid); + /* Fall through to examine the agg's arguments */ + } + else if (IsA(node, SubPlan)) { SubPlan *subplan = (SubPlan *) node; Plan *plan = planner_subplan_get_plan(context->root, subplan); diff --git a/src/include/optimizer/planmain.h b/src/include/optimizer/planmain.h index c4f61c1a09..6f87ee3258 100644 --- a/src/include/optimizer/planmain.h +++ b/src/include/optimizer/planmain.h @@ -113,6 +113,8 @@ extern bool innerrel_is_unique(PlannerInfo *root, */ extern Plan *set_plan_references(PlannerInfo *root, Plan *plan); extern bool trivial_subqueryscan(SubqueryScan *plan); +extern Param *find_minmax_agg_replacement_param(PlannerInfo *root, + Aggref *aggref); extern void record_plan_function_dependency(PlannerInfo *root, Oid funcid); extern void record_plan_type_dependency(PlannerInfo *root, Oid typid); extern bool extract_query_dependencies_walker(Node *node, PlannerInfo *root); diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index 26031bc780..6b3b9f103d 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1249,6 +1249,37 @@ NOTICE: drop cascades to 3 other objects DETAIL: drop cascades to table minmaxtest1 drop cascades to table minmaxtest2 drop cascades to table minmaxtest3 +-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465) +begin; +set local enable_sort = off; +explain (costs off) + select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1) + from int4_tbl t0; + QUERY PLAN +--------------------------------------------------------------------- + Seq Scan on int4_tbl t0 + SubPlan 2 + -> HashAggregate + Group Key: $1 + InitPlan 1 (returns $1) + -> Limit + -> Seq Scan on int4_tbl t1 + Filter: ((f1 IS NOT NULL) AND (f1 = t0.f1)) + -> Result +(9 rows) + +select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1) +from int4_tbl t0; + f1 | min +-------------+------------- + 0 | 0 + 123456 | 123456 + -123456 | -123456 + 2147483647 | 2147483647 + -2147483647 | -2147483647 +(5 rows) + +rollback; -- check for correct detection of nested-aggregate errors select max(min(unique1)) from tenk1; ERROR: aggregate function calls cannot be nested diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 48bea8af5f..b89cf26c5a 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -431,6 +431,16 @@ select distinct min(f1), max(f1) from minmaxtest; drop table minmaxtest cascade; +-- DISTINCT can also trigger wrong answers with hash aggregation (bug #18465) +begin; +set local enable_sort = off; +explain (costs off) + select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1) + from int4_tbl t0; +select f1, (select distinct min(t1.f1) from int4_tbl t1 where t1.f1 = t0.f1) +from int4_tbl t0; +rollback; + -- check for correct detection of nested-aggregate errors select max(min(unique1)) from tenk1; select (select max(min(unique1)) from int8_tbl) from tenk1;