From 7e1870a9c691849098b70fa40511d781161196ac Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Tue, 26 May 2026 15:27:21 +0900 Subject: [PATCH v1] Fix HAVING-to-WHERE pushdown with mismatched operator families When the HAVING clause uses a comparison operator from a different btree/hash opfamily than the GROUP BY's equality operator, the planner's optimization of moving HAVING clauses into WHERE can produce incorrect query results. Two values that GROUP BY considers equal (and therefore puts in the same group) can be unequal under the HAVING operator's opfamily. Pushing the clause to WHERE then filters individual rows before grouping, dropping rows that GROUP BY would have merged and changing aggregate results. Fix by detecting opfamily conflicts before flatten_group_exprs, while the HAVING clause still contains GROUP Vars (Vars referencing RTE_GROUP). At each comparison-bearing node we examine direct operands that are GROUP Vars and reject the clause if the operator's opfamily isn't equality-compatible with the SortGroupClause's eqop. The conflicting clause indices are joined with those returned by find_having_collation_conflicts and consulted during the existing HAVING-to-WHERE loop, so unaffected clauses in the same query are still pushed. Back-patch to v18 only. The fix relies on the RTE_GROUP mechanism introduced in v18 (commit 247dea89f), which is what lets us identify grouping expressions via GROUP Vars on pre-flatten havingQual. Pre-v18 branches lack that machinery, so a back-patch there would need a different approach. Given the absence of field reports of this bug on back branches, the risk of carrying a different fix on stable branches is not justified. --- src/backend/optimizer/plan/planner.c | 244 +++++++++++++++++++++-- src/backend/utils/cache/lsyscache.c | 9 +- src/test/regress/expected/aggregates.out | 74 ++++++- src/test/regress/sql/aggregates.sql | 42 +++- src/tools/pgindent/typedefs.list | 1 + 5 files changed, 334 insertions(+), 36 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index f4689e7c9f8..3a598ac6974 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -148,6 +148,15 @@ typedef struct List *ancestor_collids; } having_collation_ctx; +/* + * Context for the find_having_opfamily_conflicts walker. + */ +typedef struct +{ + Query *parse; + Index group_rtindex; +} having_opfamily_ctx; + /* Local functions */ static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind); static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode); @@ -155,6 +164,13 @@ static Bitmapset *find_having_collation_conflicts(Query *parse, Index group_rtindex); static bool having_collation_conflict_walker(Node *node, having_collation_ctx *ctx); +static Bitmapset *find_having_opfamily_conflicts(Query *parse, + Index group_rtindex); +static bool having_opfamily_conflict_walker(Node *node, + having_opfamily_ctx *ctx); +static bool comparison_has_group_eqop_conflict(Oid opno, List *args, + having_opfamily_ctx *ctx); +static Oid group_var_eqop(Query *parse, Var *var); static void grouping_planner(PlannerInfo *root, double tuple_fraction, SetOperationStmt *setops); static grouping_sets_data *preprocess_grouping_sets(PlannerInfo *root); @@ -780,7 +796,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, PlannerInfo *root; List *newWithCheckOptions; List *newHaving; - Bitmapset *havingCollationConflicts; + Bitmapset *havingPushdownConflicts; int havingIdx; bool hasOuterJoins; bool hasResultRTEs; @@ -1196,25 +1212,47 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, } /* - * Before we flatten GROUP Vars, check which HAVING clauses have collation - * conflicts. When GROUP BY uses a nondeterministic collation, values - * that are "equal" for grouping may be distinguishable under a different - * collation. If such a HAVING clause were moved to WHERE, it would - * filter individual rows before grouping, potentially eliminating some - * members of a group and thereby changing aggregate results. + * Before we flatten GROUP Vars, identify HAVING clauses whose equality + * semantics disagree with the GROUP BY's -- either via collation or + * operator family. If pushed to WHERE, such a clause would filter + * individual rows before grouping happens, eliminating rows that GROUP BY + * would have merged into a single group and thereby changing aggregate + * results. * - * We do this check before flatten_group_exprs because we can easily + * Two hazards are detected here: + * + * (1) Collation conflicts. When GROUP BY uses a nondeterministic + * collation, values that are "equal" for grouping may be distinguishable + * under a different collation applied by the HAVING clause. + * + * (2) Opfamily conflicts. A type can belong to multiple btree opfamilies + * whose equality operators disagree on which values are equal. The + * canonical example is the record type, where record_ops (the default) + * compares per-field while record_image_ops compares bytewise. Two + * records that record_ops considers equal -- and which GROUP BY therefore + * puts in the same group -- can be unequal under record_image_ops. + * + * We do these checks before flatten_group_exprs because we can easily * identify grouping expressions by checking whether a Var references * RTE_GROUP, and such Vars directly carry the GROUP BY collation as their - * varcollid. After flattening, these Vars are replaced by the underlying - * expressions, and we would have to match expressions in the HAVING - * clause back to grouping expressions, which is much more complex. + * varcollid and let us recover the SortGroupClause via varattno. After + * flattening, these Vars are replaced by the underlying expressions, and + * we would have to match expressions in the HAVING clause back to + * grouping expressions, which is much more complex. */ if (parse->hasGroupRTE) - havingCollationConflicts = - find_having_collation_conflicts(parse, root->group_rtindex); + { + Bitmapset *coll_conflicts; + Bitmapset *opf_conflicts; + + coll_conflicts = find_having_collation_conflicts(parse, + root->group_rtindex); + opf_conflicts = find_having_opfamily_conflicts(parse, + root->group_rtindex); + havingPushdownConflicts = bms_join(coll_conflicts, opf_conflicts); + } else - havingCollationConflicts = NULL; + havingPushdownConflicts = NULL; /* * Replace any Vars in the subquery's targetlist and havingQual that @@ -1260,11 +1298,11 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, * but it's okay: it's just an optimization to avoid running pull_varnos * when there cannot be any Vars in the HAVING clause.) * - * We also cannot do this if the HAVING clause uses a different collation - * than the GROUP BY for any grouping expression whose GROUP BY collation - * is nondeterministic. This is detected before flatten_group_exprs (see - * find_having_collation_conflicts above) and recorded in the - * havingCollationConflicts bitmapset. The bitmapset indexes remain valid + * We also cannot do this for HAVING clauses that conflict with GROUP BY + * on collation or operator family. Both kinds of conflict are detected + * before flatten_group_exprs (see find_having_collation_conflicts and + * find_having_opfamily_conflicts above) and recorded in the + * havingPushdownConflicts bitmapset. The bitmapset indexes remain valid * here because flatten_group_exprs uses expression_tree_mutator, which * preserves the list length and ordering of havingQual. * @@ -1308,7 +1346,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, if (contain_agg_clause(havingclause) || contain_volatile_functions(havingclause) || contain_subplans(havingclause) || - bms_is_member(havingIdx, havingCollationConflicts) || + bms_is_member(havingIdx, havingPushdownConflicts) || (parse->groupClause && parse->groupingSets && bms_is_member(root->group_rtindex, pull_varnos(root, havingclause)))) { @@ -1727,6 +1765,172 @@ having_collation_conflict_walker(Node *node, having_collation_ctx *ctx) return result; } +/* + * find_having_opfamily_conflicts + * Identify HAVING clauses that must not be moved to WHERE because they + * apply a comparison operator from a different btree/hash opfamily than + * the GROUP BY uses for that grouping column. + * + * A type may belong to multiple btree opfamilies whose equality operators + * disagree (e.g., record_ops "=" vs record_image_ops "*="). Two values + * that GROUP BY considers equal under one opfamily can be unequal under + * another, so pushing such a HAVING clause to WHERE would filter individual + * rows before grouping and change aggregate results. + * + * Like find_having_collation_conflicts, this must be called before + * flatten_group_exprs, while the HAVING clause still contains GROUP Vars. + * + * Returns a Bitmapset of zero-based indexes into the havingQual list for + * clauses that have opfamily conflicts and must stay in HAVING. + */ +static Bitmapset * +find_having_opfamily_conflicts(Query *parse, Index group_rtindex) +{ + Bitmapset *result = NULL; + having_opfamily_ctx ctx; + int idx; + + if (parse->havingQual == NULL) + return NULL; + + ctx.parse = parse; + ctx.group_rtindex = group_rtindex; + + idx = 0; + foreach_ptr(Node, clause, (List *) parse->havingQual) + { + if (having_opfamily_conflict_walker(clause, &ctx)) + result = bms_add_member(result, idx); + idx++; + } + + return result; +} + +/* + * Walker function for find_having_opfamily_conflicts. + * + * Inspect every comparison-bearing node; if any direct operand (ignoring + * RelabelType wrappers) is a GROUP Var, verify that the node's operator is + * opfamily-compatible with the grouping eqop. + */ +static bool +having_opfamily_conflict_walker(Node *node, having_opfamily_ctx *ctx) +{ + if (node == NULL) + return false; + + if (IsA(node, OpExpr)) + { + OpExpr *opexpr = (OpExpr *) node; + + if (comparison_has_group_eqop_conflict(opexpr->opno, opexpr->args, ctx)) + return true; + } + else if (IsA(node, ScalarArrayOpExpr)) + { + ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) node; + + if (comparison_has_group_eqop_conflict(saop->opno, saop->args, ctx)) + return true; + } + else if (IsA(node, RowCompareExpr)) + { + RowCompareExpr *rcexpr = (RowCompareExpr *) node; + ListCell *lc_l; + ListCell *lc_r; + ListCell *lc_o; + + /* Each column has its own opno; check it against direct GROUP Vars. */ + forthree(lc_l, rcexpr->largs, + lc_r, rcexpr->rargs, + lc_o, rcexpr->opnos) + { + Oid opno = lfirst_oid(lc_o); + List *pair = list_make2(lfirst(lc_l), lfirst(lc_r)); + bool conflict; + + conflict = comparison_has_group_eqop_conflict(opno, pair, ctx); + list_free(pair); + if (conflict) + return true; + } + } + + return expression_tree_walker(node, having_opfamily_conflict_walker, ctx); +} + +/* + * comparison_has_group_eqop_conflict + * For each Var operand of a comparison that references RTE_GROUP, check + * that 'opno' is equality-compatible with the grouping eqop. + * + * Strips RelabelType wrappers so const-folded CollateExpr leaves don't hide + * the underlying Var. Operators not in any indexed opfamily are skipped. + */ +static bool +comparison_has_group_eqop_conflict(Oid opno, List *args, + having_opfamily_ctx *ctx) +{ + ListCell *lc; + + if (!OidIsValid(opno) || !op_is_safe_index_member(opno)) + return false; + + foreach(lc, args) + { + Node *arg = (Node *) lfirst(lc); + Var *var; + Oid group_eqop; + + if (arg && IsA(arg, RelabelType)) + arg = (Node *) ((RelabelType *) arg)->arg; + + if (arg == NULL || !IsA(arg, Var)) + continue; + + var = (Var *) arg; + if (var->varno != ctx->group_rtindex || var->varlevelsup != 0) + continue; + + group_eqop = group_var_eqop(ctx->parse, var); + if (!equality_ops_are_compatible(opno, group_eqop)) + return true; + } + + return false; +} + +/* + * group_var_eqop + * Return the equality operator that GROUP BY uses for the given GROUP Var. + * + * A GROUP Var's varattno is its 1-based position in the RTE_GROUP's groupexprs + * list, which addRangeTableEntryForGroup built by iterating parse->groupClause + * and including every SortGroupClause whose TLE was present in the targetlist. + * Replay that traversal here to recover the SortGroupClause for the given + * varattno. + */ +static Oid +group_var_eqop(Query *parse, Var *var) +{ + int counter = 0; + + Assert(var->varlevelsup == 0); + + foreach_node(SortGroupClause, sgc, parse->groupClause) + { + if (get_sortgroupclause_tle(sgc, parse->targetList) == NULL) + continue; + if (++counter == var->varattno) + return sgc->eqop; + } + + elog(ERROR, "could not find GROUP clause for GROUP Var attno %d", + var->varattno); + return InvalidOid; /* keep compiler quiet */ +} + /* * preprocess_phv_expression * Do preprocessing on a PlaceHolderVar expression that's been pulled up. diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c index 036086057d7..51a084332c7 100644 --- a/src/backend/utils/cache/lsyscache.c +++ b/src/backend/utils/cache/lsyscache.c @@ -902,10 +902,11 @@ collations_agree_on_equality(Oid coll1, Oid coll2) * op_is_safe_index_member * Check if the operator is a member of a B-tree or Hash operator family. * - * We use this check as a proxy for "null-safety": if an operator is trusted by - * the btree or hash opfamily, it implies that the operator adheres to standard - * boolean behavior, and would not return NULL when given valid non-null - * inputs, as doing so would break index integrity. + * Membership in such an opfamily has several useful implications: the operator + * returns non-null for non-null inputs (i.e. "null-safety", required so that + * the operator doesn't break index integrity), and it agrees with other + * members of the same opfamily on equality semantics. Callers use this check + * as a proxy for any of those properties. */ bool op_is_safe_index_member(Oid opno) diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index fbda0e3bbc2..ed966016ebe 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1645,11 +1645,20 @@ explain (costs off) select y,z from t2 group by y,z; -> Seq Scan on t2 (3 rows) +drop table t1 cascade; +NOTICE: drop cascades to table t1c +drop table t2; +drop table t3; +drop table p_t1; +-- A composite type used by the tests below to exercise the asymmetry +-- between record_ops (per-field equality, the default) and record_image_ops +-- (bytewise equality): values like row(1.0) and row(1.00) are field-equal +-- but byte-distinct. +create type t_rec as (x numeric); -- A unique index proves uniqueness only under its own opfamily. When the -- GROUP BY's eqop comes from a different opfamily with looser equality, -- rows the index regards as distinct can collapse into one GROUP BY group, -- so the index is not usable for removing redundant columns. -create type t_rec as (x numeric); create temp table t_opf (a t_rec not null, b text); create unique index on t_opf (a record_image_ops); -- (1.0) and (1.00) are bytewise distinct but logically equal as records; @@ -1675,12 +1684,65 @@ select a, b from t_opf group by a, b order by b; (2 rows) drop table t_opf; +-- A HAVING clause that uses an equality operator from a different opfamily +-- than the GROUP BY's eqop must NOT be pushed down to WHERE. +create temp table t_having (id int, a t_rec); +insert into t_having values + (1, row(1.0)::t_rec), + (2, row(1.00)::t_rec), + (3, row(2)::t_rec); +-- the clause must stay in HAVING +explain (costs off) +select a, count(*) from t_having group by a having a *= row(1.0)::t_rec; + QUERY PLAN +--------------------------------- + HashAggregate + Group Key: a + Filter: (a *= '(1.0)'::t_rec) + -> Seq Scan on t_having +(4 rows) + +select a, count(*) from t_having group by a having a *= row(1.0)::t_rec; + a | count +-------+------- + (1.0) | 2 +(1 row) + +-- the clause must stay in HAVING +explain (costs off) +select a, count(*) from t_having group by a having a *= any (array[row(1.0)::t_rec]); + QUERY PLAN +------------------------------------------- + HashAggregate + Group Key: a + Filter: (a *= ANY ('{(1.0)}'::t_rec[])) + -> Seq Scan on t_having +(4 rows) + +select a, count(*) from t_having group by a having a *= any (array[row(1.0)::t_rec]); + a | count +-------+------- + (1.0) | 2 +(1 row) + +-- the clause can be pushed down to WHERE +explain (costs off) +select a, count(*) from t_having group by a having a = row(1.0)::t_rec; + QUERY PLAN +-------------------------------------- + GroupAggregate + -> Seq Scan on t_having + Filter: (a = '(1.0)'::t_rec) +(3 rows) + +select a, count(*) from t_having group by a having a = row(1.0)::t_rec; + a | count +-------+------- + (1.0) | 2 +(1 row) + +drop table t_having; drop type t_rec; -drop table t1 cascade; -NOTICE: drop cascades to table t1c -drop table t2; -drop table t3; -drop table p_t1; -- -- Test GROUP BY ALL -- diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 580f364ba97..1945d006e5f 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -577,11 +577,21 @@ alter table t2 alter column z drop not null; create unique index t2_z_uidx on t2(z) nulls not distinct; explain (costs off) select y,z from t2 group by y,z; +drop table t1 cascade; +drop table t2; +drop table t3; +drop table p_t1; + +-- A composite type used by the tests below to exercise the asymmetry +-- between record_ops (per-field equality, the default) and record_image_ops +-- (bytewise equality): values like row(1.0) and row(1.00) are field-equal +-- but byte-distinct. +create type t_rec as (x numeric); + -- A unique index proves uniqueness only under its own opfamily. When the -- GROUP BY's eqop comes from a different opfamily with looser equality, -- rows the index regards as distinct can collapse into one GROUP BY group, -- so the index is not usable for removing redundant columns. -create type t_rec as (x numeric); create temp table t_opf (a t_rec not null, b text); create unique index on t_opf (a record_image_ops); -- (1.0) and (1.00) are bytewise distinct but logically equal as records; @@ -592,12 +602,32 @@ explain (costs off) select a, b from t_opf group by a, b order by b; select a, b from t_opf group by a, b order by b; drop table t_opf; -drop type t_rec; -drop table t1 cascade; -drop table t2; -drop table t3; -drop table p_t1; +-- A HAVING clause that uses an equality operator from a different opfamily +-- than the GROUP BY's eqop must NOT be pushed down to WHERE. +create temp table t_having (id int, a t_rec); +insert into t_having values + (1, row(1.0)::t_rec), + (2, row(1.00)::t_rec), + (3, row(2)::t_rec); + +-- the clause must stay in HAVING +explain (costs off) +select a, count(*) from t_having group by a having a *= row(1.0)::t_rec; +select a, count(*) from t_having group by a having a *= row(1.0)::t_rec; + +-- the clause must stay in HAVING +explain (costs off) +select a, count(*) from t_having group by a having a *= any (array[row(1.0)::t_rec]); +select a, count(*) from t_having group by a having a *= any (array[row(1.0)::t_rec]); + +-- the clause can be pushed down to WHERE +explain (costs off) +select a, count(*) from t_having group by a having a = row(1.0)::t_rec; +select a, count(*) from t_having group by a having a = row(1.0)::t_rec; + +drop table t_having; +drop type t_rec; -- -- Test GROUP BY ALL diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 8cf40c87043..d2e864b390c 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3856,6 +3856,7 @@ gss_name_t gtrgm_consistent_cache gzFile having_collation_ctx +having_opfamily_ctx heap_page_items_state help_handler hlCheck -- 2.39.5 (Apple Git-154)