From eec8e6f9bf53fbd983f982d4f1589a637ca7434f Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Wed, 19 Nov 2025 16:38:44 +0900 Subject: [PATCH v2] Strip PlaceHolderVars from index operands When pulling up a subquery, we may need to wrap its targetlist items in PlaceHolderVars to enforce separate identity or as a result of outer joins. However, this causes any upper-level WHERE clauses referencing these outputs to contain PlaceHolderVars, which prevents indxpath.c from recognizing that they could be matched to index columns or index expressions, potentially affecting the planner's ability to use indexes. To fix, explicitly strip PlaceHolderVars from index operands. A PlaceHolderVar appearing in a relation-scan-level expression is effectively a no-op. Nevertheless, to play it safe, we strip only PlaceHolderVars that are not marked nullable and whose syntactic scope matches the index. The stripping is performed recursively to handle cases where PlaceHolderVars are nested or interleaved with other node types. To minimize performance impact, we first use a lightweight walker to check for the presence of strippable PlaceHolderVars. The expensive mutator is invoked only if a candidate is found, avoiding unnecessary memory allocation and tree copying in the common case where no PlaceHolderVars are present. --- src/backend/optimizer/path/indxpath.c | 125 ++++++++++++++++++++- src/backend/optimizer/plan/createplan.c | 14 ++- src/include/optimizer/paths.h | 1 + src/test/regress/expected/groupingsets.out | 64 +++++++++++ src/test/regress/sql/groupingsets.sql | 33 ++++++ src/tools/pgindent/typedefs.list | 1 + 6 files changed, 229 insertions(+), 9 deletions(-) diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index 2654c59c4c6..34b7040cb63 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -75,6 +75,12 @@ typedef struct int indexcol; /* index column we want to match to */ } ec_member_matches_arg; +/* Context for the walker/mutator for stripping PHVs in index operand */ +typedef struct +{ + Relids index_relids; +} StripPHVContext; + static void consider_index_join_clauses(PlannerInfo *root, RelOptInfo *rel, IndexOptInfo *index, @@ -4359,12 +4365,23 @@ match_index_to_operand(Node *operand, int indkey; /* - * Ignore any RelabelType node above the operand. This is needed to be - * able to apply indexscanning in binary-compatible-operator cases. Note: - * we can assume there is at most one RelabelType node; - * eval_const_expressions() will have simplified if more than one. + * Ignore any PlaceHolderVar node contained in the operand. This is + * needed to be able to apply indexscanning in cases where the operand (or + * a subtree) has been wrapped in PlaceHolderVars to enforce separate + * identity or as a result of outer joins. + */ + operand = strip_phvs_in_index_operand(operand, index->rel->relids); + + /* + * Ignore any RelabelType node above the operand. This is needed to be + * able to apply indexscanning in binary-compatible-operator cases. + * + * Note: we must handle nested RelabelType nodes here. While + * eval_const_expressions() will have simplified them to at most one + * layer, our prior stripping of PlaceHolderVars may have brought separate + * RelabelTypes into adjacency. */ - if (operand && IsA(operand, RelabelType)) + while (operand && IsA(operand, RelabelType)) operand = (Node *) ((RelabelType *) operand)->arg; indkey = index->indexkeys[indexcol]; @@ -4417,6 +4434,104 @@ match_index_to_operand(Node *operand, return false; } +/* + * contain_strippable_phv_walker + * Detect if there are any PlaceHolderVars in the tree that are candidates + * for stripping. + * + * We identify a PlaceHolderVar as strippable only if its phnullingrels is + * empty and its syntactic scope matches the target index. + */ +static bool +contain_strippable_phv_walker(Node *node, void *context) +{ + StripPHVContext *ctx = (StripPHVContext *) context; + + if (node == NULL) + return false; + + if (IsA(node, PlaceHolderVar)) + { + PlaceHolderVar *phv = (PlaceHolderVar *) node; + + if (bms_is_empty(phv->phnullingrels) && + bms_equal(phv->phrels, ctx->index_relids)) + return true; + } + + return expression_tree_walker(node, contain_strippable_phv_walker, + context); +} + +/* + * strip_phvs_in_index_operand_mutator + * Recursively remove PlaceHolderVars in the tree that match the criteria. + * + * We strip a PlaceHolderVar only if its phnullingrels is empty and its + * syntactic scope matches the target index, replacing it with its contained + * expression. + */ +static Node * +strip_phvs_in_index_operand_mutator(Node *node, void *context) +{ + StripPHVContext *ctx = (StripPHVContext *) context; + + if (node == NULL) + return NULL; + + if (IsA(node, PlaceHolderVar)) + { + PlaceHolderVar *phv = (PlaceHolderVar *) node; + + /* If matches the criteria, strip it */ + if (bms_is_empty(phv->phnullingrels) && + bms_equal(phv->phrels, ctx->index_relids)) + { + /* Recurse on its contained expression */ + return strip_phvs_in_index_operand_mutator((Node *) phv->phexpr, + context); + } + + /* Otherwise, keep this PHV but check its contained expression */ + } + + return expression_tree_mutator(node, strip_phvs_in_index_operand_mutator, + context); +} + +/* + * strip_phvs_in_index_operand + * Strip PlaceHolderVar nodes from the given operand expression to + * facilitate matching against an index's key. + * + * A PlaceHolderVar appearing in a relation-scan-level expression is + * effectively a no-op. Nevertheless, to play it safe, we strip only + * PlaceHolderVars that are not marked nullable and whose syntactic scope + * matches this index. + * + * The removal is performed recursively because PlaceHolderVars can be nested + * or interleaved with other node types. We must peel back all layers to + * expose the base operand. + * + * As a performance optimization, we first use a lightweight walker to check + * for the presence of strippable PlaceHolderVars. The expensive mutator is + * invoked only if a candidate is found, avoiding unnecessary memory allocation + * and tree copying in the common case where no PlaceHolderVars are present. + */ +Node * +strip_phvs_in_index_operand(Node *operand, Relids index_relids) +{ + StripPHVContext ctx; + + ctx.index_relids = index_relids; + + /* Don't mutate/copy if no target PHVs exist */ + if (!contain_strippable_phv_walker(operand, (void *) &ctx)) + return operand; + + return strip_phvs_in_index_operand_mutator(operand, (void *) &ctx); +} + /* * is_pseudo_constant_for_index() * Test whether the given expression can be used as an indexscan diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 8af091ba647..eca58069b55 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -5100,7 +5100,8 @@ fix_indexqual_clause(PlannerInfo *root, IndexOptInfo *index, int indexcol, * equal to the index's attribute number (index column position). * * Most of the code here is just for sanity cross-checking that the given - * expression actually matches the index column it's claimed to. + * expression actually matches the index column it's claimed to. It should + * match the logic in match_index_to_operand(). */ static Node * fix_indexqual_operand(Node *node, IndexOptInfo *index, int indexcol) @@ -5109,14 +5110,19 @@ fix_indexqual_operand(Node *node, IndexOptInfo *index, int indexcol) int pos; ListCell *indexpr_item; + Assert(indexcol >= 0 && indexcol < index->ncolumns); + + /* + * Remove any PlaceHolderVar wrapping of the indexkey + */ + node = strip_phvs_in_index_operand(node, index->rel->relids); + /* * Remove any binary-compatible relabeling of the indexkey */ - if (IsA(node, RelabelType)) + while (IsA(node, RelabelType)) node = (Node *) ((RelabelType *) node)->arg; - Assert(indexcol >= 0 && indexcol < index->ncolumns); - if (index->indexkeys[indexcol] != 0) { /* It's a simple index column */ diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index f6a62df0b43..41405a98d82 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -81,6 +81,7 @@ extern bool indexcol_is_bool_constant_for_query(PlannerInfo *root, int indexcol); extern bool match_index_to_operand(Node *operand, int indexcol, IndexOptInfo *index); +extern Node *strip_phvs_in_index_operand(Node *operand, Relids index_relids); extern void check_index_predicates(PlannerInfo *root, RelOptInfo *rel); /* diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index 39d35a195bc..01e8d518d61 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -463,6 +463,70 @@ select x, y || 'y' | 3y (8 rows) +-- check that operands wrapped in PlaceHolderVars are capable of index matching +begin; +set local enable_bitmapscan = off; +explain (costs off) +select x, y + from (select unique1 as x, unique2 as y from tenk1) as t + where x = 1 + group by grouping sets (x, y) + order by 1, 2; + QUERY PLAN +----------------------------------------------------- + Sort + Sort Key: tenk1.unique1, tenk1.unique2 + -> GroupAggregate + Group Key: tenk1.unique1 + Sort Key: tenk1.unique2 + Group Key: tenk1.unique2 + -> Index Scan using tenk1_unique1 on tenk1 + Index Cond: (unique1 = 1) +(8 rows) + +select x, y + from (select unique1 as x, unique2 as y from tenk1) as t + where x = 1 + group by grouping sets (x, y) + order by 1, 2; + x | y +---+------ + 1 | + | 2838 +(2 rows) + +explain (costs off) +select x, y + from (select unique1::oid as x, unique2 as y from tenk1) as t + where x::integer = 1 + group by grouping sets (x, y) + order by 1, 2; + QUERY PLAN +----------------------------------------------------------------- + Sort + Sort Key: ((tenk1.unique1)::oid), tenk1.unique2 + -> GroupAggregate + Group Key: ((tenk1.unique1)::oid) + Sort Key: tenk1.unique2 + Group Key: tenk1.unique2 + -> Sort + Sort Key: ((tenk1.unique1)::oid) + -> Index Scan using tenk1_unique1 on tenk1 + Index Cond: (((unique1)::oid)::integer = 1) +(10 rows) + +select x, y + from (select unique1::oid as x, unique2 as y from tenk1) as t + where x::integer = 1 + group by grouping sets (x, y) + order by 1, 2; + x | y +---+------ + 1 | + | 2838 +(2 rows) + +rollback; -- check qual push-down rules for a subquery with grouping sets explain (verbose, costs off) select * from ( diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql index 6d875475fae..d45cd8947fb 100644 --- a/src/test/regress/sql/groupingsets.sql +++ b/src/test/regress/sql/groupingsets.sql @@ -183,6 +183,39 @@ select x, y || 'y' group by grouping sets (x, y) order by 1, 2; +-- check that operands wrapped in PlaceHolderVars are capable of index matching +begin; + +set local enable_bitmapscan = off; + +explain (costs off) +select x, y + from (select unique1 as x, unique2 as y from tenk1) as t + where x = 1 + group by grouping sets (x, y) + order by 1, 2; + +select x, y + from (select unique1 as x, unique2 as y from tenk1) as t + where x = 1 + group by grouping sets (x, y) + order by 1, 2; + +explain (costs off) +select x, y + from (select unique1::oid as x, unique2 as y from tenk1) as t + where x::integer = 1 + group by grouping sets (x, y) + order by 1, 2; + +select x, y + from (select unique1::oid as x, unique2 as y from tenk1) as t + where x::integer = 1 + group by grouping sets (x, y) + order by 1, 2; + +rollback; + -- check qual push-down rules for a subquery with grouping sets explain (verbose, costs off) select * from ( diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 6e2ed0c8825..e597111926d 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2894,6 +2894,7 @@ StreamStopReason String StringInfo StringInfoData +StripPHVContext StripnullState SubLink SubLinkType -- 2.39.5 (Apple Git-154)