From 5e7328928a905748a0b547ef857fdd6b2f03bf10 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Tue, 23 Jun 2026 12:44:45 +0000 Subject: [PATCH 1/2] Catch grouping columns wrapped in an expression The conflict check only inspected direct Var operands, so a grouping column buried in a wrapper, such as amt::text or a reconstructed record, slipped a finer comparison past it and the qual was pushed across the grouping boundary, returning wrong results. A wrapper is opaque, but it can only split a group when the grouping equality is not "image-faithful" (equal values need not be byte-identical, as with numeric scale, record image, or float -0.0). Test that with the btree equalimage support function and reject a wrapped operand containing any such grouping column. Image-faithful columns such as integers and deterministic text cannot be split, so their wrapped quals like length(name) = 3 still push. Types with no btree opclass or no equalimage proc fall to the conservative side. --- src/backend/optimizer/util/clauses.c | 95 +++++++++++++++++++++++- src/test/regress/expected/aggregates.out | 56 ++++++++++++++ src/test/regress/sql/aggregates.sql | 25 +++++++ 3 files changed, 175 insertions(+), 1 deletion(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 4bd255cbbdf..34e1265c5c8 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -20,8 +20,13 @@ #include "postgres.h" #include "access/htup_details.h" +#include "access/nbtree.h" +#include "access/stratnum.h" #include "access/table.h" +#include "catalog/pg_am.h" +#include "catalog/pg_amop.h" #include "catalog/pg_class.h" +#include "catalog/pg_collation.h" #include "catalog/pg_inherits.h" #include "catalog/pg_language.h" #include "catalog/pg_operator.h" @@ -53,6 +58,7 @@ #include "tcop/tcopprot.h" #include "utils/acl.h" #include "utils/builtins.h" +#include "utils/catcache.h" #include "utils/datum.h" #include "utils/fmgroids.h" #include "utils/json.h" @@ -6299,6 +6305,11 @@ pull_paramids_walker(Node *node, Bitmapset **context) * equal. * * Returns true if any such conflict exists. + * + * A grouping column wrapped in an expression is handled conservatively; see + * wrapped_operand_has_grouping_conflict(). Only OpExpr / ScalarArrayOpExpr / + * RowCompareExpr are inspected, so a grouping column consumed by some other + * boolean function is not caught. */ bool expression_has_grouping_conflict(Node *expr, @@ -6489,6 +6500,77 @@ grouping_conflict_walker(Node *node, grouping_walker_ctx *ctx) return result; } +/* + * grouping_eqop_is_image_faithful + * True if 'eqop' equates only byte-identical values (btree equalimage). + * Then no wrapper can split a group on that column. When that cannot be + * proven, because there is no btree opclass, no equalimage proc, or it + * returns false (as for numeric, record or float), the eqop is reported as + * not faithful. + */ +static bool +grouping_eqop_is_image_faithful(Oid eqop) +{ + CatCList *catlist; + Oid opfamily = InvalidOid; + Oid opcintype = InvalidOid; + Oid equalimageproc; + int i; + + catlist = SearchSysCacheList1(AMOPOPID, ObjectIdGetDatum(eqop)); + for (i = 0; i < catlist->n_members; i++) + { + Form_pg_amop amop = (Form_pg_amop) GETSTRUCT(&catlist->members[i]->tuple); + + if (amop->amopmethod == BTREE_AM_OID && + amop->amopstrategy == BTEqualStrategyNumber) + { + opfamily = amop->amopfamily; + opcintype = amop->amoplefttype; + break; + } + } + ReleaseSysCacheList(catlist); + + if (!OidIsValid(opfamily)) + return false; + + equalimageproc = get_opfamily_proc(opfamily, opcintype, opcintype, + BTEQUALIMAGE_PROC); + if (!OidIsValid(equalimageproc)) + return false; + + return DatumGetBool(OidFunctionCall1Coll(equalimageproc, C_COLLATION_OID, + ObjectIdGetDatum(opcintype))); +} + +/* + * wrapped_operand_has_grouping_conflict + * True if 'node' (a wrapped comparison operand) contains a grouping column + * whose equality is not image-faithful. The wrapper may expose a + * distinction the grouping hides (e.g. amt::text reveals numeric scale), so + * we keep the qual; image-faithful columns can't be split and stay pushable. + */ +static bool +wrapped_operand_has_grouping_conflict(Node *node, grouping_walker_ctx *ctx) +{ + if (node == NULL) + return false; + + if (IsA(node, Var)) + { + Var *var = (Var *) node; + Oid eqop = ctx->get_eqop(var, ctx->cb_context); + + if (OidIsValid(eqop) && !grouping_eqop_is_image_faithful(eqop)) + return true; + return false; + } + + return expression_tree_walker(node, wrapped_operand_has_grouping_conflict, + ctx); +} + /* * comparison_has_grouping_eqop_conflict * Per-comparison helper: ask the callback whether each Var operand of @@ -6499,6 +6581,10 @@ grouping_conflict_walker(Node *node, grouping_walker_ctx *ctx) * the underlying Var. Operators not in any btree/hash opfamily are skipped * (see the header comment on op_is_safe_index_member). * + * A wrapped operand (not a bare Var) goes to + * wrapped_operand_has_grouping_conflict(): a grouping column buried in an + * expression would otherwise escape the test above and be wrongly relocated. + * * The check is symmetric: any cross-opfamily comparison is rejected. In * principle a qual operator from an opfamily whose equality is coarser than * the grouping eqop could still be applied safely, since a coarser equality @@ -6525,9 +6611,16 @@ comparison_has_grouping_eqop_conflict(Oid opno, List *args, if (arg && IsA(arg, RelabelType)) arg = (Node *) ((RelabelType *) arg)->arg; - if (arg == NULL || !IsA(arg, Var)) + if (arg == NULL) continue; + if (!IsA(arg, Var)) + { + if (wrapped_operand_has_grouping_conflict(arg, ctx)) + return true; + continue; + } + var = (Var *) arg; grouping_eqop = ctx->get_eqop(var, ctx->cb_context); if (!OidIsValid(grouping_eqop)) diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index ed966016ebe..6f653446224 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1741,8 +1741,64 @@ select a, count(*) from t_having group by a having a = row(1.0)::t_rec; (1.0) | 2 (1 row) +-- a grouping column reached through a wrapper must also stay in HAVING: +-- record_ops has no equalimage support, so a reconstructed record is unsafe +-- even though the outer "=" matches the grouping eqop. +explain (costs off) +select a, count(*) from t_having group by a + having row((a).x)::t_rec = row(1.0)::t_rec; + QUERY PLAN +------------------------------------------------ + HashAggregate + Group Key: a + Filter: (ROW((a).x)::t_rec = '(1.0)'::t_rec) + -> Seq Scan on t_having +(4 rows) + drop table t_having; drop type t_rec; +-- numeric ignores scale, so a cast to text can split a group: must stay in +-- HAVING. A wrapper over an image-faithful column (text) is safe and pushes. +create temp table t_wrap (n numeric, s text); +insert into t_wrap values (1.0, 'aa'), (1.00, 'aa'), (2, 'b'); +explain (costs off) +select n, count(*) from t_wrap group by n having n::text = '1.0'; + QUERY PLAN +------------------------------------- + HashAggregate + Group Key: n + Filter: ((n)::text = '1.0'::text) + -> Seq Scan on t_wrap +(4 rows) + +explain (costs off) +select s, count(*) from t_wrap group by s having length(s) = 2; + QUERY PLAN +--------------------------------------- + GroupAggregate + Group Key: s + -> Sort + Sort Key: s + -> Seq Scan on t_wrap + Filter: (length(s) = 2) +(6 rows) + +drop table t_wrap; +-- xid groups by hash and has no btree opclass, so a wrapped xid column falls +-- to the conservative side and stays in HAVING. +create temp table t_xid (x xid); +insert into t_xid values ('1'), ('1'), ('2'); +explain (costs off) +select x, count(*) from t_xid group by x having x::text = '1'; + QUERY PLAN +----------------------------------- + HashAggregate + Group Key: x + Filter: ((x)::text = '1'::text) + -> Seq Scan on t_xid +(4 rows) + +drop table t_xid; -- -- Test GROUP BY ALL -- diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql index 1945d006e5f..251a6d5e4ba 100644 --- a/src/test/regress/sql/aggregates.sql +++ b/src/test/regress/sql/aggregates.sql @@ -626,9 +626,34 @@ 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; +-- a grouping column reached through a wrapper must also stay in HAVING: +-- record_ops has no equalimage support, so a reconstructed record is unsafe +-- even though the outer "=" matches the grouping eqop. +explain (costs off) +select a, count(*) from t_having group by a + having row((a).x)::t_rec = row(1.0)::t_rec; + drop table t_having; drop type t_rec; +-- numeric ignores scale, so a cast to text can split a group: must stay in +-- HAVING. A wrapper over an image-faithful column (text) is safe and pushes. +create temp table t_wrap (n numeric, s text); +insert into t_wrap values (1.0, 'aa'), (1.00, 'aa'), (2, 'b'); +explain (costs off) +select n, count(*) from t_wrap group by n having n::text = '1.0'; +explain (costs off) +select s, count(*) from t_wrap group by s having length(s) = 2; +drop table t_wrap; + +-- xid groups by hash and has no btree opclass, so a wrapped xid column falls +-- to the conservative side and stays in HAVING. +create temp table t_xid (x xid); +insert into t_xid values ('1'), ('1'), ('2'); +explain (costs off) +select x, count(*) from t_xid group by x having x::text = '1'; +drop table t_xid; + -- -- Test GROUP BY ALL -- -- 2.43.0