From e1f42b1f8b1bc7e9128220f5517235004f510a87 Mon Sep 17 00:00:00 2001 From: Andrey Rachitskiy Date: Fri, 4 Sep 2026 20:41:04 +0500 Subject: [PATCH v3] 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. Also check GROUP BY in qual_is_pushdown_safe, so a wrapped jsonb qual is not pushed below aggregation. Bug: #19649 Author: Andrey Rachitskiy Reviewed-by: Andrei Lepikhov Reported-by: chunling qin <303677365@qq.com> --- src/backend/optimizer/path/allpaths.c | 52 ++++++----- src/backend/optimizer/util/clauses.c | 152 +++++++++++++++++++++++++----- src/test/regress/expected/subselect.out | 160 ++++++++++++++++++++++++++++++++ src/test/regress/sql/subselect.sql | 70 ++++++++++++++ 4 files changed, 387 insertions(+), 47 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 24a6a8d11dd..29556ca857e 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -4445,15 +4445,15 @@ targetIsInAllPartitionLists(TargetEntry *tle, Query *query) * 5. rinfo's clause must not refer to any subquery output columns that were * found to be unsafe to reference by subquery_is_pushdown_safe(). * - * 6. If the subquery has a grouping layer (DISTINCT, DISTINCT ON, window - * PARTITION BY, or a set operation that groups rows by equality), rinfo's - * clause must not apply a different equivalence relation to a grouping column - * than the grouping uses; otherwise it would distinguish rows the grouping - * considers equal, and pushing such a clause past the grouping would drop - * members of a group and change which row becomes the group's representative - * (or, for window functions, change per-partition values such as ranks and - * counts). See expression_has_grouping_conflict for the kinds of conflict - * detected. + * 6. If the subquery has a grouping layer (GROUP BY, DISTINCT, DISTINCT ON, + * window PARTITION BY, or a set operation that groups rows by equality), + * rinfo's clause must not apply a different equivalence relation to a + * grouping column than the grouping uses. Otherwise it would distinguish + * rows the grouping considers equal, and pushing such a clause past the + * grouping would drop members of a group and change which row becomes the + * group's representative (or, for window functions, change per-partition + * values such as ranks and counts). See expression_has_grouping_conflict + * for the kinds of conflict detected. */ static pushdown_safe_type qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo, @@ -4552,7 +4552,8 @@ qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo, /* Check point 6 */ if (safe == PUSHDOWN_SAFE && - (subquery->hasWindowFuncs || + (subquery->groupClause != NIL || + subquery->hasWindowFuncs || subquery->distinctClause != NIL || (subquery->setOperations != NULL && setop_has_grouping(subquery->setOperations)))) @@ -4579,20 +4580,11 @@ static Oid pushdown_var_grouping_eqop(Var *var, void *context) { Query *subquery = (Query *) context; - Oid eqop; if (var->varlevelsup != 0) return InvalidOid; - eqop = subquery_column_grouping_eqop(subquery, var->varattno); - - /* - * qual_is_pushdown_safe ensures any level-0 subquery Var that reaches us - * references a grouping column. - */ - Assert(OidIsValid(eqop)); - - return eqop; + return subquery_column_grouping_eqop(subquery, var->varattno); } /* @@ -4602,11 +4594,12 @@ pushdown_var_grouping_eqop(Var *var, void *context) * participate in any grouping mechanism. * * A subquery output column is grouping-relevant if it appears in - * subquery->distinctClause (covering both DISTINCT and DISTINCT ON), in every - * window's PARTITION BY clause, or is grouped by some node in a set-operation - * tree. In all of these cases the parser builds the SortGroupClause with the - * column's type-default equality operator via get_sort_group_operators, so any - * matching SortGroupClause carries the correct eqop. + * subquery->groupClause, subquery->distinctClause (covering both DISTINCT and + * DISTINCT ON), in every window's PARTITION BY clause, or is grouped by some + * node in a set-operation tree. In all of these cases the parser builds the + * SortGroupClause with the column's type-default equality operator via + * get_sort_group_operators, so any matching SortGroupClause carries the + * correct eqop. Aggregate output columns are not grouping-relevant. */ static Oid subquery_column_grouping_eqop(Query *subquery, AttrNumber attno) @@ -4619,6 +4612,15 @@ subquery_column_grouping_eqop(Query *subquery, AttrNumber attno) tle = list_nth_node(TargetEntry, subquery->targetList, attno - 1); + /* GROUP BY */ + foreach(lc, subquery->groupClause) + { + SortGroupClause *sgc = lfirst_node(SortGroupClause, lc); + + if (sgc->tleSortGroupRef == tle->ressortgroupref) + return sgc->eqop; + } + /* DISTINCT or DISTINCT ON */ foreach(lc, subquery->distinctClause) { diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 55cebe4a74b..f161b38e702 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,103 @@ 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; + + /* jsonb only. The same check on every type would block safe wrappers. */ + 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 +6784,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..01228fe574d 100644 --- a/src/test/regress/expected/subselect.out +++ b/src/test/regress/expected/subselect.out @@ -2151,6 +2151,166 @@ 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 + Filter: ((s.j)::text = '1'::text) + -> HashAggregate + Group Key: pdt_json.j + -> 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 + Filter: ((s.j)::text = '1.0'::text) + -> HashAggregate + Group Key: pdt_json.j + -> 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) + +-- Same GROUP BY shape with hash aggregation disabled. That is the +-- GroupAggregate plan that still pushed the wrapper onto Seq Scan +-- without the GROUP BY check in qual_is_pushdown_safe. +CREATE TEMP TABLE pdt_json_pk (id int primary key, j jsonb); +INSERT INTO pdt_json_pk VALUES (1, '1'), (2, '1.0'); +SET enable_hashagg TO off; +EXPLAIN (COSTS OFF) +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1'; + QUERY PLAN +------------------------------------------- + Subquery Scan on s + Filter: ((s.j)::text = '1'::text) + -> GroupAggregate + Group Key: pdt_json_pk.j + -> Sort + Sort Key: pdt_json_pk.j + -> Seq Scan on pdt_json_pk +(7 rows) + +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1'; + j | c +---+--- + 1 | 2 +(1 row) + +EXPLAIN (COSTS OFF) +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1.0'; + QUERY PLAN +------------------------------------------- + Subquery Scan on s + Filter: ((s.j)::text = '1.0'::text) + -> GroupAggregate + Group Key: pdt_json_pk.j + -> Sort + Sort Key: pdt_json_pk.j + -> Seq Scan on pdt_json_pk +(7 rows) + +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1.0'; + j | c +---+--- +(0 rows) + +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j = '1'::jsonb; + j | c +---+--- + 1 | 2 +(1 row) + +RESET enable_hashagg; +-- 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..99d63f669d2 100644 --- a/src/test/regress/sql/subselect.sql +++ b/src/test/regress/sql/subselect.sql @@ -1052,6 +1052,76 @@ 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; + +-- Same GROUP BY shape with hash aggregation disabled. That is the +-- GroupAggregate plan that still pushed the wrapper onto Seq Scan +-- without the GROUP BY check in qual_is_pushdown_safe. +CREATE TEMP TABLE pdt_json_pk (id int primary key, j jsonb); +INSERT INTO pdt_json_pk VALUES (1, '1'), (2, '1.0'); +SET enable_hashagg TO off; +EXPLAIN (COSTS OFF) +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1'; + +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1'; + +EXPLAIN (COSTS OFF) +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1.0'; + +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j::text = '1.0'; + +SELECT j, c FROM (SELECT j, count(*) c FROM pdt_json_pk GROUP BY j) s +WHERE j = '1'::jsonb; +RESET enable_hashagg; + +-- 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