nocfbot-0001-Catch-grouping-columns-wrapped-in-an-express.patch

From 5e7328928a905748a0b547ef857fdd6b2f03bf10 Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <zsolt.parragi@percona.com>
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