From e1f42b1f8b1bc7e9128220f5517235004f510a87 Mon Sep 17 00:00:00 2001 From: Andrey Rachitskiy Date: Fri, 4 Sep 2026 20:41:04 +0500 Subject: [PATCH v2] Fix qual pushdown for wrapped grouping comparisons A grouping column used through a wrapper inside a comparison operand can apply a finer equivalence relation than the grouping boundary uses. For example, jsonb grouping merges 1 and 1.0, but wrappers such as j::text or j #>> '{}' can separate them when pushed below DISTINCT or GROUP BY. Keep the direct-operand checks unchanged, and recurse into non-direct comparison operands only for wrapped jsonb grouping Vars. Add regression cases for wrapped jsonb comparisons, and a guard case that confirms a safe wrapper qual (i::text = '5') is still pushed down to Seq Scan. Bug: #19649 Author: Andrey Rachitskiy Reviewed-by: Andrei Lepikhov Reported-by: chunling qin <303677365@qq.com> --- src/backend/optimizer/util/clauses.c | 156 ++++++++++++++++++++---- src/test/regress/expected/subselect.out | 105 ++++++++++++++++ src/test/regress/sql/subselect.sql | 46 +++++++ 3 files changed, 285 insertions(+), 22 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 55cebe4a74b..73738679d59 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -27,6 +27,7 @@ #include "catalog/pg_operator.h" #include "catalog/pg_proc.h" #include "catalog/pg_type.h" +#include "catalog/pg_type_d.h" #include "executor/executor.h" #include "executor/functions.h" #include "funcapi.h" @@ -112,6 +113,9 @@ typedef struct grouping_eqop_callback get_eqop; void *cb_context; Var *case_var; + Oid cmp_opno; + Oid cmp_inputcollid; + bool cmp_context_valid; } grouping_walker_ctx; static bool contain_agg_clause_walker(Node *node, void *context); @@ -133,6 +137,15 @@ static List *find_nonnullable_vars_walker(Node *node, bool top_level); static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK); static bool convert_saop_to_hashed_saop_walker(Node *node, void *context); static bool grouping_conflict_walker(Node *node, grouping_walker_ctx *ctx); +static bool grouping_var_has_nondeterministic_collation(Var *var); +static bool grouping_var_has_comparison_conflict(Var *var, Oid opno, + Oid inputcollid, + grouping_walker_ctx *ctx); +static bool grouping_operand_has_comparison_conflict(Node *node, Oid opno, + Oid inputcollid, + grouping_walker_ctx *ctx); +static bool grouping_operand_has_comparison_conflict_walker(Node *node, + void *context); static bool grouping_check_operands(Oid opno, Oid inputcollid, List *args, grouping_walker_ctx *ctx); static bool grouping_check_operand(Node *arg, Oid opno, Oid inputcollid, @@ -6417,15 +6430,13 @@ pull_paramids_walker(Node *node, Bitmapset **context) * For a nondeterministic collation, every other reference is rejected: a * comparison under a different collation, and any function or operator over * the column, because we cannot tell whether the function yields the same - * result for values the grouping treats as equal, and many do not. A column - * with a deterministic collation is not restricted this way. + * result for values the grouping treats as equal, and many do not. * - * This leaves one case uncaught: with a deterministic collation, a function - * over the column can still feed a finer comparison than the direct-operand - * check sees, for example record_image_ops over a rebuilt record, or scale() - * over numeric where two equal values differ in scale. Catching it would - * require knowing that a type's equality is bitwise, which we do not test - * here. + * In addition, within a comparison, if an operand is not a direct grouping + * Var, we recurse into it and apply the same opfamily/collation checks to + * wrapped jsonb grouping Vars found there. This catches wrappers such as + * CoerceViaIO and text-extraction operators over jsonb that can feed a + * comparison using a different equality relation than grouping does. * * Returns true if any such conflict exists. */ @@ -6468,6 +6479,13 @@ expression_has_grouping_conflict(Node *expr, * ArrayCoerceExpr's elemexpr and a JsonConstructorExpr's coercion, which * stand for something else. */ +static bool +grouping_var_has_nondeterministic_collation(Var *var) +{ + return OidIsValid(var->varcollid) && + !get_collation_isdeterministic(var->varcollid); +} + static bool grouping_conflict_walker(Node *node, grouping_walker_ctx *ctx) { @@ -6487,8 +6505,7 @@ grouping_conflict_walker(Node *node, grouping_walker_ctx *ctx) * boolean is not collatable, so it takes the deterministic path here. */ if (OidIsValid(ctx->get_eqop(var, ctx->cb_context)) && - OidIsValid(var->varcollid) && - !get_collation_isdeterministic(var->varcollid)) + grouping_var_has_nondeterministic_collation(var)) return true; return false; } @@ -6642,6 +6659,107 @@ grouping_check_operands(Oid opno, Oid inputcollid, List *args, return false; } +/* + * grouping_var_has_comparison_conflict + * Apply direct-operand grouping checks to one Var. + * + * Returns true when this Var is a grouping column and the surrounding + * comparison would apply a conflicting equivalence relation, either because + * the comparison operator is from an incompatible equality family or because + * a nondeterministic-collation grouping Var is compared under a different + * collation. + */ +static bool +grouping_var_has_comparison_conflict(Var *var, Oid opno, Oid inputcollid, + grouping_walker_ctx *ctx) +{ + Oid grouping_eqop = ctx->get_eqop(var, ctx->cb_context); + + if (!OidIsValid(grouping_eqop)) + return false; + + if (!equality_ops_are_compatible(opno, grouping_eqop)) + return true; + + if (grouping_var_has_nondeterministic_collation(var) && + inputcollid != var->varcollid) + return true; + + return false; +} + +/* + * grouping_operand_has_comparison_conflict + * Recursively inspect a non-direct comparison operand. + * + * grouping_check_operand calls this only after determining that the operand is + * not itself a direct Var reference. + */ +static bool +grouping_operand_has_comparison_conflict(Node *node, Oid opno, + Oid inputcollid, + grouping_walker_ctx *ctx) +{ + grouping_walker_ctx check_ctx = *ctx; + + check_ctx.cmp_opno = opno; + check_ctx.cmp_inputcollid = inputcollid; + check_ctx.cmp_context_valid = true; + + return grouping_operand_has_comparison_conflict_walker(node, &check_ctx); +} + +/* + * grouping_operand_has_comparison_conflict_walker + * Walker for grouping_operand_has_comparison_conflict. + * + * 'context' is grouping_walker_ctx with cmp_* fields set for the surrounding + * comparison. We descend through wrapper structure and apply + * grouping_var_has_comparison_conflict to every grouping Var found in the + * operand subtree. CaseTestExpr is resolved through ctx->case_var, + * matching the CASE handling used by grouping_conflict_walker. + * + * Returns true if any wrapped jsonb grouping Var inside this operand would + * fail the same operator/collation checks that we use for direct operands. + */ +static bool +grouping_operand_has_comparison_conflict_walker(Node *node, void *context) +{ + grouping_walker_ctx *ctx = (grouping_walker_ctx *) context; + + if (node == NULL) + return false; + + if (IsA(node, RelabelType)) + return grouping_operand_has_comparison_conflict_walker( + (Node *) ((RelabelType *) node)->arg, context); + + if (IsA(node, CaseTestExpr)) + return grouping_operand_has_comparison_conflict_walker( + (Node *) ctx->case_var, context); + + if (IsA(node, Var)) + { + Var *var = (Var *) node; + + /* + * Keep this check narrow: v1's all-type recursion blocked safe + * pushdown such as int4->text wrappers. The known broken class here + * is wrappers over jsonb grouping keys. + */ + if (getBaseType(var->vartype) != JSONBOID) + return false; + + Assert(ctx->cmp_context_valid); + return grouping_var_has_comparison_conflict(var, ctx->cmp_opno, + ctx->cmp_inputcollid, ctx); + } + + return expression_tree_walker(node, + grouping_operand_has_comparison_conflict_walker, + context); +} + /* * grouping_check_operand * Handle one operand 'arg' of a comparison with operator 'opno' and @@ -6670,22 +6788,16 @@ grouping_check_operand(Node *arg, Oid opno, Oid inputcollid, if (node && IsA(node, Var)) { Var *var = (Var *) node; - Oid grouping_eqop = ctx->get_eqop(var, ctx->cb_context); - if (OidIsValid(grouping_eqop)) - { - /* incompatible equality semantics */ - if (!equality_ops_are_compatible(opno, grouping_eqop)) - return true; - /* nondeterministic collation compared under a different collation */ - if (OidIsValid(var->varcollid) && - !get_collation_isdeterministic(var->varcollid) && - inputcollid != var->varcollid) - return true; - } + if (grouping_var_has_comparison_conflict(var, opno, inputcollid, ctx)) + return true; return false; /* direct operand handled; do not recurse */ } + /* Recurse into non-direct operands and reuse direct checks. */ + if (grouping_operand_has_comparison_conflict(arg, opno, inputcollid, ctx)) + return true; + return grouping_conflict_walker(arg, ctx); } diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out index cf295d56507..ae3e76b856f 100644 --- a/src/test/regress/expected/subselect.out +++ b/src/test/regress/expected/subselect.out @@ -2151,6 +2151,111 @@ WHERE (CASE id WHEN 1 THEN 1 ELSE 0 END) = 1; Filter: (CASE id WHEN 1 THEN 1 ELSE 0 END = 1) (5 rows) +-- Wrapped jsonb comparisons over grouped subqueries (DISTINCT ON and +-- GROUP BY): wrapped quals must stay above grouping. +CREATE TEMP TABLE pdt_json (id int, j jsonb); +INSERT INTO pdt_json VALUES + (1, '1'), + (2, '1.0'); +EXPLAIN (COSTS OFF) +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j::text = '1.0'; + QUERY PLAN +------------------------------------------------- + Subquery Scan on s + Filter: ((s.j)::text = '1.0'::text) + -> Unique + -> Sort + Sort Key: pdt_json.j, pdt_json.id + -> Seq Scan on pdt_json +(6 rows) + +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j::text = '1.0'; + id | j +----+--- +(0 rows) + +EXPLAIN (COSTS OFF) +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j #>> '{}' = '1.0'; + QUERY PLAN +-------------------------------------------------- + Subquery Scan on s + Filter: ((s.j #>> '{}'::text[]) = '1.0'::text) + -> Unique + -> Sort + Sort Key: pdt_json.j, pdt_json.id + -> Seq Scan on pdt_json +(6 rows) + +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j #>> '{}' = '1.0'; + id | j +----+--- +(0 rows) + +EXPLAIN (COSTS OFF) +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1'; + QUERY PLAN +-------------------------------------------------- + Subquery Scan on s + -> HashAggregate + Group Key: pdt_json.j + Filter: ((pdt_json.j)::text = '1'::text) + -> Seq Scan on pdt_json +(5 rows) + +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1'; + c +--- + 2 +(1 row) + +EXPLAIN (COSTS OFF) +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1.0'; + QUERY PLAN +---------------------------------------------------- + Subquery Scan on s + -> HashAggregate + Group Key: pdt_json.j + Filter: ((pdt_json.j)::text = '1.0'::text) + -> Seq Scan on pdt_json +(5 rows) + +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1.0'; + c +--- +(0 rows) + +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j = '1'::jsonb; + c +--- + 2 +(1 row) + +-- But safe wrappers such as int4->text must still push below grouping. +CREATE TEMP TABLE pdt_int (i int, s text, ts timestamptz); +SET enable_hashagg TO off; +EXPLAIN (COSTS OFF) +SELECT * FROM (SELECT i, count(*) c FROM pdt_int GROUP BY i) s +WHERE i::text = '5'; + QUERY PLAN +----------------------------------------------- + GroupAggregate + Group Key: pdt_int.i + -> Sort + Sort Key: pdt_int.i + -> Seq Scan on pdt_int + Filter: ((i)::text = '5'::text) +(6 rows) + +RESET enable_hashagg; -- Set operations: any operation other than UNION ALL groups rows by equality, -- so the same opfamily-mismatch rules apply. CREATE TEMP TABLE u1 (a t_rec); diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql index 07438694f6e..01937ae249c 100644 --- a/src/test/regress/sql/subselect.sql +++ b/src/test/regress/sql/subselect.sql @@ -1052,6 +1052,52 @@ EXPLAIN (COSTS OFF) SELECT * FROM (SELECT DISTINCT id FROM pdt) s WHERE (CASE id WHEN 1 THEN 1 ELSE 0 END) = 1; +-- Wrapped jsonb comparisons over grouped subqueries (DISTINCT ON and +-- GROUP BY): wrapped quals must stay above grouping. +CREATE TEMP TABLE pdt_json (id int, j jsonb); +INSERT INTO pdt_json VALUES + (1, '1'), + (2, '1.0'); + +EXPLAIN (COSTS OFF) +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j::text = '1.0'; + +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j::text = '1.0'; + +EXPLAIN (COSTS OFF) +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j #>> '{}' = '1.0'; + +SELECT * FROM (SELECT DISTINCT ON (j) id, j FROM pdt_json ORDER BY j, id) s +WHERE j #>> '{}' = '1.0'; + +EXPLAIN (COSTS OFF) +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1'; + +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1'; + +EXPLAIN (COSTS OFF) +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1.0'; + +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j::text = '1.0'; + +SELECT c FROM (SELECT j, count(*) c FROM pdt_json GROUP BY j) s +WHERE j = '1'::jsonb; + +-- But safe wrappers such as int4->text must still push below grouping. +CREATE TEMP TABLE pdt_int (i int, s text, ts timestamptz); +SET enable_hashagg TO off; +EXPLAIN (COSTS OFF) +SELECT * FROM (SELECT i, count(*) c FROM pdt_int GROUP BY i) s +WHERE i::text = '5'; +RESET enable_hashagg; + -- Set operations: any operation other than UNION ALL groups rows by equality, -- so the same opfamily-mismatch rules apply. CREATE TEMP TABLE u1 (a t_rec); -- 2.53.0