From 7d7dce051f78341d9be430814b479cbdcaa11202 Mon Sep 17 00:00:00 2001 From: Andrey Rachitskiy Date: Sat, 5 Sep 2026 14:59:32 +0500 Subject: [PATCH v4] 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. jsonb grouping merges 1 and 1.0, but a wrapper such as j::text or j #>> '{}' can tell them apart. Pushing such a qual below DISTINCT or GROUP BY then splits one group and changes counts and group keys. Keep the direct-operand checks unchanged, and recurse into non-direct comparison operands, applying the same operator and collation checks to wrapped jsonb grouping Vars found there. The walker is shared by both grouping boundaries, so this one change is enough. qual_is_pushdown_safe uses it for DISTINCT, window PARTITION BY and set operations. For GROUP BY the pushed qual goes to HAVING in subquery_push_qual, and find_having_conflicts uses the same walker to keep it there instead of lowering it to WHERE. Add regression cases for wrapped jsonb comparisons under DISTINCT ON and GROUP BY, and a guard that a safe wrapper such as 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 | 158 ++++++++++++++++++++---- src/test/regress/expected/subselect.out | 105 ++++++++++++++++ src/test/regress/sql/subselect.sql | 46 +++++++ 3 files changed, 287 insertions(+), 22 deletions(-) diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 55cebe4a74b..77df991af8f 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,109 @@ 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; + + /* + * Restrict this to jsonb. jsonb equality treats 1 and 1.0 as equal, + * so a wrapper such as ::text or #>> can tell apart values the + * grouping merged. Applying the check to every type would also + * reject wrappers that preserve the grouping equivalence, such as + * int4 to text, and block their safe pushdown. + */ + 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 +6790,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