pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
16+ messages / 3 participants
[nested] [flat]
* GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-11-17 08:02 Haowu Ge <gehaowu@bitmoe.com>
0 siblings, 1 reply; 16+ messages in thread
From: Haowu Ge @ 2025-11-17 08:02 UTC (permalink / raw)
To: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hello everyone,
Recently, after upgrading my database from PostgreSQL 16 to 18, I encountered an issue: when performing a GROUP BY ROLLUP on a view, the query planner resorts to a full table scan instead of using the index on the underlying table. This severely impacts performance.
----------
:-( The task lasted for 20 seconds, and in the end, it ran for 3.6 hours, Under the same environment and query instructions, pg16 before the upgrade did not have this issue
Planning:
Buffers: shared hit=1829 read=67
I/O Timings: shared read=309.026
Planning Time: 344.548 ms
Execution Time: 12999763.259 ms
----------
Below is a step-by-step reproduction of the issue:
# 1. Create a base table 'materials' to store product information.
CREATE TABLE materials (
material_id VARCHAR PRIMARY KEY,
description TEXT NOT NULL,
unit_price NUMERIC(10, 2) NOT NULL
);
# 2. Create an explicit index on 'material_id'.
CREATE INDEX idx_material_id ON materials(material_id);
# 3. Create a simple view that mirrors the base table structure.
CREATE VIEW materials_view AS
SELECT material_id, description, unit_price FROM materials;
# 4. Insert test data:
INSERT INTO materials (material_id, description, unit_price)
VALUES ('PI', 'Example Product', 99.99);
INSERT INTO materials (material_id, description, unit_price)
SELECT
'MAT' || generate_series(1,49999)::TEXT,
'Description of product ' || generate_series(1,49999)::TEXT,
(random() * 1000 + 1)::NUMERIC(10,2);
# 5. Query the base table to compute the average unit price for material 'PI',using a simple GROUP BY (no rollup)
EXPLAIN ANALYZE
SELECT material_id, AVG(unit_price) as avg_price
FROM materials
WHERE material_id = 'PI'
GROUP BY material_id;
# 6. Query the base table to compute the average unit price for material 'PI',using a simple GROUP BY (rollup)
EXPLAIN ANALYZE
SELECT material_id, AVG(unit_price) as avg_price
FROM materials
WHERE material_id = 'PI'
GROUP BY ROLLUP(material_id);
# 7. Query the view table to compute the average unit price for material 'PI',using a simple GROUP BY (no rollup)
EXPLAIN ANALYZE
SELECT material_id, AVG(unit_price) as avg_price
FROM materials_view
WHERE material_id = 'PI'
GROUP BY
material_id;
# 8. Query the view table to compute the average unit price for material 'PI',using a simple GROUP BY (rollup)
EXPLAIN ANALYZE
SELECT material_id, AVG(unit_price) as avg_price
FROM materials_view
WHERE material_id = 'PI'
GROUP BY ROLLUP(material_id);
Log, please refer to the attachment for more logs:
GroupAggregate (cost=0.00..1097.39 rows=251 width=64) (actual time=3.983..3.985 rows=2.00 loops=1)
Group Key: materials.material_id
Group Key: ()
Buffers: shared hit=468
-> Seq Scan on materials (cost=0.00..1093.00 rows=250 width=48) (actual time=0.018..3.970 rows=1.00 loops=1)
Filter: ((material_id)::text = 'PI'::text)
Rows Removed by Filter: 49999
Buffers: shared hit=468
Planning Time: 0.117 ms
Execution Time: 4.029 ms
Thanks & Best Regards
_________________________________________________________________________________
Haowu Ge (BG5FRG) | Homepage: https://www.gehaowu.com <https://www.gehaowu.com > | PGP:7A06 1F6E DF09 D8A8
Attachments:
[application/octet-stream] view_rollup_bug_EXPLAIN_ANALYZE.txt (3.8K, ../../62af586c-c270-44f3-9c5e-02c81d537e3d.gehaowu@bitmoe.com/3-view_rollup_bug_EXPLAIN_ANALYZE.txt)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-11-18 08:27 Richard Guo <guofenglinux@gmail.com>
parent: Haowu Ge <gehaowu@bitmoe.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-11-18 08:27 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On Mon, Nov 17, 2025 at 5:02 PM Haowu Ge <gehaowu@bitmoe.com> wrote:
> # 8. Query the view table to compute the average unit price for material 'PI',using a simple GROUP BY (rollup)
> EXPLAIN ANALYZE
> SELECT material_id, AVG(unit_price) as avg_price
> FROM materials_view
> WHERE material_id = 'PI'
> GROUP BY ROLLUP(material_id);
> GroupAggregate (cost=0.00..1097.39 rows=251 width=64) (actual time=3.983..3.985 rows=2.00 loops=1)
> Group Key: materials.material_id
> Group Key: ()
> Buffers: shared hit=468
> -> Seq Scan on materials (cost=0.00..1093.00 rows=250 width=48) (actual time=0.018..3.970 rows=1.00 loops=1)
> Filter: ((material_id)::text = 'PI'::text)
> Rows Removed by Filter: 49999
> Buffers: shared hit=468
> Planning Time: 0.117 ms
> Execution Time: 4.029 ms
Thanks for the report. The reason why this query cannot use an index
scan is that, when the parent query uses grouping sets, all subquery
outputs are wrapped in PlaceHolderVars. This is to ensure that these
expressions retain their separate identity so that they can match to
the expected grouping set columns (cf. cc5d98525). However, it also
prevents the WHERE clause from matching the index column, which is why
the index cannot be used.
Prior to v18, we wrapped only the subquery outputs that were non-Var
expressions. That is why this query was able to use an index scan.
However, that behavior could produce incorrect results when the
subquery's targetlist contained two or more identical Var expressions.
In addition, if the targetlist included any non-Var expressions, we
could also fail to match the WHERE clause to the corresponding index
expressions.
After looking at this query more closely, it seems that there is no
need to wrap the expressions in the WHERE clause in the first place.
It should be sufficient to wrap only the expressions that are used
logically above the aggregation step, ie, those in the targetlist and
in the havingQual. However, pullup_replace_vars currently lacks the
infrastructure to do that.
Another possible fix is to detect whether the subquery's targetlist
contains two or more identical Var expressions and, if not, fall back
to wrapping only the non-Var expressions. This would restore the
behavior that existed prior to v18 for this query.
Alternatively, we could teach match_index_to_operand to unwrap PHV
expressions when matching them to indexes. This would also allow the
WHERE clause to match indexes for non-Var expressions, which is an
improvement over the behavior prior to v18. But I haven't fully
thought through whether this approach is safe in all cases,
particularly when outer joins are involved.
Any thoughts?
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-11-19 10:07 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-11-19 10:07 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On Tue, Nov 18, 2025 at 5:27 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Alternatively, we could teach match_index_to_operand to unwrap PHV
> expressions when matching them to indexes. This would also allow the
> WHERE clause to match indexes for non-Var expressions, which is an
> improvement over the behavior prior to v18. But I haven't fully
> thought through whether this approach is safe in all cases,
> particularly when outer joins are involved.
I believe this approach is safe, because a PlaceHolderVar appearing in
a relation-scan-level expression is effectively a no-op. To play it
safe, we could verify that the PlaceHolderVar is not marked nullable
and that its syntactic scope matches this index, and only strip such
PlaceHolderVars (though it's unclear if this is really necessary).
Attached is a patch that implements this approach. With this patch,
the reported query can use an index scan again. Additionally, it
enables planner to explore index scans in other cases that were
previously not possible, even before v18. For example:
create table t (a int);
insert into t select i from generate_series(1,1000)i;
create index on t (coalesce(a, 1));
analyze t;
explain (costs off)
select * from t t1 left join
(select coalesce(a, 1) as c, * from t t2) s on true
where t1.a > s.c;
QUERY PLAN
-----------------------------------------------
Nested Loop
-> Seq Scan on t t1
-> Index Scan using t_coalesce_idx on t t2
Index Cond: (COALESCE(a, 1) < t1.a)
(4 rows)
The expression coalesce(a, 1) is wrapped in a PlaceHolderVar, and
previously it was not possible to match it to the index expression.
- Richard
Attachments:
[application/octet-stream] v1-0001-Strip-PlaceHolderVars-from-index-operands.patch (6.5K, ../../CAMbWs48J51_SJ5nyA5P=d0mtkwvXf3rS=nOC7HL5+hjrbPZuSw@mail.gmail.com/2-v1-0001-Strip-PlaceHolderVars-from-index-operands.patch)
download | inline diff:
From 460f18ce96684ee844243eca28e8a79bed1143d8 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Nov 2025 16:38:44 +0900
Subject: [PATCH v1] 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. This is
safe because a PlaceHolderVar appearing in a relation-scan-level
expression is effectively a no-op.
---
src/backend/optimizer/path/indxpath.c | 29 ++++++++++++++++++-
src/backend/optimizer/plan/createplan.c | 23 +++++++++++++--
src/test/regress/expected/groupingsets.out | 33 ++++++++++++++++++++++
src/test/regress/sql/groupingsets.sql | 20 +++++++++++++
4 files changed, 101 insertions(+), 4 deletions(-)
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index c62e3f87724..bf07119939f 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -4349,7 +4349,34 @@ match_index_to_operand(Node *operand,
int indkey;
/*
- * Ignore any RelabelType node above the operand. This is needed to be
+ * Ignore any PlaceHolderVar node above the operand. This is needed to be
+ * able to apply indexscanning in cases where the operand has been wrapped
+ * in PlaceHolderVars to enforce separate identity or as a result of outer
+ * joins. Note: there may be multiple layers of PlaceHolderVar nodes, as
+ * one PlaceHolderVar can be wrapped inside another.
+ *
+ * It's safe because a PlaceHolderVar appearing in a relation-scan-level
+ * expression is effectively a no-op.
+ *
+ * To be on the safe side, punt if we encounter a PlaceHolderVar that is
+ * marked nullable or whose syntactic scope is beyond this index. XXX Is
+ * it possible to have such a PlaceHolderVar at the relation-scan level?
+ */
+ while (operand && IsA(operand, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) operand;
+ int varno;
+
+ if (bms_is_empty(phv->phnullingrels) &&
+ bms_get_singleton_member(phv->phrels, &varno) &&
+ varno == index->rel->relid)
+ operand = (Node *) phv->phexpr;
+ else
+ return false;
+ }
+
+ /*
+ * 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.
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 8af091ba647..99413d7187c 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,30 @@ 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
+ */
+ while (IsA(node, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+ int varno;
+
+ if (bms_is_empty(phv->phnullingrels) &&
+ bms_get_singleton_member(phv->phrels, &varno) &&
+ varno == index->rel->relid)
+ node = (Node *) phv->phexpr;
+ else
+ elog(ERROR, "index key does not match expected index column");
+ }
+
/*
* Remove any binary-compatible relabeling of the indexkey
*/
if (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/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 398cf6965e0..f3af6cb5797 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -463,6 +463,39 @@ 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)
+
+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..3ea5b03e79d 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -183,6 +183,26 @@ 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;
+
+rollback;
+
-- check qual push-down rules for a subquery with grouping sets
explain (verbose, costs off)
select * from (
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-08 06:10 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-08 06:10 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Dec 5, 2025 at 11:17 AM Haowu Ge <gehaowu@bitmoe.com> wrote:
> I tried applying the patch myself, compiled and installed it,
> but it doesn’t seem to resolve the issue of GROUP BY ROLLUP leading to a full table scan.
I see why that failed. In your example scenario, the operand ends up
as RelabelType(PlaceHolderVar(Var)). The v1 patch attempted to strip
PlaceHolderVar nodes first and then RelabelType nodes. So the outer
RelabelType blocked the logic from seeing and stripping the inner
PlaceHolderVar, causing the match to fail.
You may think we can reverse the stripping order to fix this, but we
cannot, because these nodes can be interleaved in arbitrary ways. For
instance:
CREATE TABLE t (a int, b int);
CREATE INDEX ON t (a);
SELECT * FROM (SELECT a::oid AS x FROM t)
WHERE x::int = 1
GROUP BY ROLLUP(x);
The operand is RT(PHV(RT(Var))). To handle such interleaving, we may
need a loop that repeatedly peels off both node types from the top of
the stack until the underlying operand is revealed.
However, even a robust top-level loop is insufficient because PHVs can
be buried deeply within an expression tree. This is particularly
problematic for expression indexes. Consider an index on (a+1) with
the following query:
CREATE INDEX ON t ((a+1));
SELECT * FROM (SELECT a AS x FROM t)
WHERE x + 1 = 2
GROUP BY ROLLUP(x);
In this case, the operand becomes OpExpr(+, PHV(Var), Const(1)). The
top-level stripping cannot reach this nested PHV node, which causes
the match to fail at the end, because match_index_to_operand() relies
on equal() to verify the match.
Therefore, we may need to recurse into the operand expression tree to
perform a deep strip of the PHVs, leveraging expression_tree_mutator.
A significant concern with this deep-strip approach is the performance
cost. match_index_to_operand() lies on the planner's hot path, and
unconditionally invoking expression_tree_mutator for every check would
introduce unacceptable overhead: that involves memory allocation and
tree copying.
To mitigate this, we can avoid the stripping logic when there are no
PHVs to remove. We can check root->glob->lastPHId == 0 to determine
if there's no PHVs at all. Unfortunately, the "root" parameter is not
available in match_index_to_operand(), and modifying the function
signature to pass it down would break ABI compatibility, which should
not happen in a patch for back-patching. (We can do that on master
though.)
As a viable alternative, maybe we can use a lightweight, read-only
expression walker on the operand tree first to detect the presence of
any PHVs. We only trigger the expensive deep mutation if this
preliminary check confirms that it is actually necessary.
I'm wondering whether we need to deep-strip RelabelType nodes as well.
Is it possible to have a case where the operand OpExpr(+, RT(a), 1)
fails to match an index expression OpExpr(+, a, 1)? Even if so, maybe
we do not need to deep-strip RelabelType nodes. There is a distinct
difference between these two node types: PHVs are artifacts injected
purely by the planner and users have no control over them, while
RelabelType nodes usually reflect explicit types or casts derived from
the user's query. If the user defines the index as (a::text || b),
they are expected to write WHERE (a::text || b) = ....
Any thoughts?
Hi Tom, I wonder if you can provide some insights?
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-09 06:38 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-09 06:38 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Mon, Dec 8, 2025 at 3:10 PM Richard Guo <guofenglinux@gmail.com> wrote:
> As a viable alternative, maybe we can use a lightweight, read-only
> expression walker on the operand tree first to detect the presence of
> any PHVs. We only trigger the expensive deep mutation if this
> preliminary check confirms that it is actually necessary.
Here is an updated patch along these lines.
Haowu, could you please verify if it fixes your query?
- Richard
Attachments:
[application/octet-stream] v2-0001-Strip-PlaceHolderVars-from-index-operands.patch (12.3K, ../../CAMbWs4-jteRUuZ63p3yyHG6=ggV9a3cNj_vMuYBrURhyXdMg_w@mail.gmail.com/2-v2-0001-Strip-PlaceHolderVars-from-index-operands.patch)
download | inline diff:
From eec8e6f9bf53fbd983f982d4f1589a637ca7434f Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
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)
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-09 08:54 Haowu Ge <gehaowu@bitmoe.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Haowu Ge @ 2025-12-09 08:54 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
Hello Richard,
I'm running PostgreSQL using the PGDG Docker image.
I built it via dpkg-buildpackage,
but I'm unable to generate a proper installation package with the v2 patch (I'll provide specific details shortly).
If I directly replace the binary in the Docker container with ./build/tmp_install/usr/lib/postgresql/18/bin/postgres, the container fails to start properly.
---
apt-get source "postgresql-18"
cd postgresql-*
patch -p1 < ~/v2-0001-Strip-PlaceHolderVars-from-index-operands.patch
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_resetwal'
make -C pg_rewind checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_rewind'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_rewind'
make -C pg_test_fsync checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_test_fsync'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_test_fsync'
make -C pg_test_timing checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_test_timing'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_test_timing'
make -C pg_upgrade checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_upgrade'
for extra in contrib/test_decoding; do make -C '../../..'/$extra DESTDIR='/root/CNPROJ/pg18.1/postgresql-18-18.1/build'/tmp_install install || exit; done
make[4]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/contrib/test_decoding'
/bin/mkdir -p '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/tmp_install/usr/lib/postgresql/18/lib'
/usr/bin/install -c -m 755 test_decoding.so '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/tmp_install/usr/lib/postgresql/18/lib/'
/bin/mkdir -p '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/tmp_install/usr/lib/postgresql/18/lib/bitcode/test_decoding'
/bin/mkdir -p '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/tmp_install/usr/lib/postgresql/18/lib/bitcode'/test_decoding/
/usr/bin/install -c -m 644 test_decoding.bc '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/tmp_install/usr/lib/postgresql/18/lib/bitcode'/test_decoding/./
cd '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/tmp_install/usr/lib/postgresql/18/lib/bitcode' && /usr/lib/llvm-19/bin/llvm-lto -thinlto -thinlto-action=thinlink -o test_decoding.index.bc test_decoding/test_decoding.bc
make[4]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/contrib/test_decoding'
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_upgrade'
make -C pg_verifybackup checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_verifybackup'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_verifybackup'
make -C pg_waldump checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_waldump'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_waldump'
make -C pg_walsummary checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_walsummary'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pg_walsummary'
make -C pgbench checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pgbench'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/pgbench'
make -C psql checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/psql'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/psql'
make -C scripts checkprep
make[3]: Entering directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/scripts'
make[3]: Nothing to be done for 'checkprep'.
make[3]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin/scripts'
make[2]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build/src/bin'
make[1]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1/build'
make[1]: *** [/usr/share/postgresql-common/server/postgresql.mk:280: override_dh_auto_test-arch] Error 1
make[1]: Leaving directory '/root/CNPROJ/pg18.1/postgresql-18-18.1'
make: *** [/usr/share/postgresql-common/server/postgresql.mk:173: binary] Error 2
dpkg-buildpackage: error: debian/rules binary subprocess returned exit status 2
root@debian:~/CNPROJ/pg18.1/postgresql-18-18.1# find . -name postgres
./build/src/backend/postgres
./build/tmp_install/usr/lib/postgresql/18/lib/bitcode/postgres
./build/tmp_install/usr/lib/postgresql/18/bin/postgres
root@debian:~/CNPROJ/pg18.1/postgresql-18-18.1# du -h ./build/tmp_install/usr/lib/postgresql/18/bin/postgres
50M ./build/tmp_install/usr/lib/postgresql/18/bin/postgres
root@debian:~/CNPROJ/pg18.1/postgresql-18-18.1#
---
Thanks & Best Regards
_________________________________________________________________________________
Haowu Ge (BG5FRG) | Homepage: https://www.gehaowu.com <https://www.gehaowu.com > | PGP:7A06 1F6E DF09 D8A8
------------------------------------------------------------------
发件人：Richard Guo <guofenglinux@gmail.com>
发送时间：2025年12月9日(周二) 14:38
收件人："葛豪武"<gehaowu@bitmoe.com>
抄 送："pgsql-bugs"<pgsql-bugs@lists.postgresql.org>; Tom Lane<tgl@sss.pgh.pa.us>
主 题：Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
On Mon, Dec 8, 2025 at 3:10 PM Richard Guo <guofenglinux@gmail.com> wrote:
> As a viable alternative, maybe we can use a lightweight, read-only
> expression walker on the operand tree first to detect the presence of
> any PHVs. We only trigger the expensive deep mutation if this
> preliminary check confirms that it is actually necessary.
Here is an updated patch along these lines.
Haowu, could you please verify if it fixes your query?
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-10 02:01 Richard Guo <guofenglinux@gmail.com>
parent: Haowu Ge <gehaowu@bitmoe.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-10 02:01 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Tue, Dec 9, 2025 at 5:54 PM Haowu Ge <gehaowu@bitmoe.com> wrote:
> I built it via dpkg-buildpackage,
> but I'm unable to generate a proper installation package with the v2 patch
Hmm, I don't have much insight into the installation issue. I tested
the v2 patch on both v18 and master, and it fixes your query.
EXPLAIN (COSTS OFF)
SELECT material_id, AVG(unit_price) as avg_price
FROM materials_view
WHERE material_id = 'PI'
GROUP BY ROLLUP(material_id);
QUERY PLAN
--------------------------------------------------------
GroupAggregate
Group Key: materials.material_id
Group Key: ()
-> Index Scan using materials_pkey on materials
Index Cond: ((material_id)::text = 'PI'::text)
(5 rows)
Regarding back-patching, I believe this issue exists before v18, but
it seems that the change in v18 made it common enough to notice,
especially in queries with grouping sets. Given the lack of reports
for versions prior to v18, I'm inclined to back-patch this only to
v18.
Any thoughts?
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-11 02:32 Haowu Ge <gehaowu@bitmoe.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Haowu Ge @ 2025-12-11 02:32 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
Hello Richard,
> Hmm, I don't have much insight into the installation issue. I tested
> the v2 patch on both v18 and master, and it fixes your query.
Based on the "Minimal Reproducible Example" principle,
I previously created a simplified version of the example,
which you successfully fixed. However, during actual testing,
I found that the behavior does not meet expectations.
Specifically, I observed inconsistent index behavior when using a view compared to querying the underlying table directly.
To help illustrate this issue, I used DeepSeek to generate a test case that closely mirrors my real-world database scenario.
As you can see:
*
Approach 1 Does not use a view and uses GROUP BY ROLLUP. uses the date column as the indexed filtering field.
Recheck Cond: (((document_date >= '2024-06-01'::date) AND (document_date <= '2024-12-31'::date)) OR ((document_date >= '2025-01-01'::date) AND (document_date <= '2025-12-09'::date)))
*
Approach 2 Uses a view and uses GROUP BY ROLLUP. uses columns "approval_status" and "data_control_scope" as indexed filtering fields, which unfortunately causes the query to scan all partitions of my partitioned table.
Recheck Cond: (((approval_status)::text = 'y'::text) AND ((data_control_scope)::text = 'DD_OG'::text))
Please refer to the attached file for the detailed example.
Thanks & Best Regards
_________________________________________________________________________________
Haowu Ge (BG5FRG) | Homepage: https://www.gehaowu.com <https://www.gehaowu.com > | PGP:7A06 1F6E DF09 D8A8
------------------------------------------------------------------
发件人：Richard Guo <guofenglinux@gmail.com>
发送时间：2025年12月10日(周三) 10:02
收件人："葛豪武"<gehaowu@bitmoe.com>
抄 送："pgsql-bugs"<pgsql-bugs@lists.postgresql.org>; Tom Lane<tgl@sss.pgh.pa.us>
主 题：Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
On Tue, Dec 9, 2025 at 5:54 PM Haowu Ge <gehaowu@bitmoe.com> wrote:
> I built it via dpkg-buildpackage,
> but I'm unable to generate a proper installation package with the v2 patch
Hmm, I don't have much insight into the installation issue. I tested
the v2 patch on both v18 and master, and it fixes your query.
EXPLAIN (COSTS OFF)
SELECT material_id, AVG(unit_price) as avg_price
FROM materials_view
WHERE material_id = 'PI'
GROUP BY ROLLUP(material_id);
QUERY PLAN
--------------------------------------------------------
GroupAggregate
Group Key: materials.material_id
Group Key: ()
-> Index Scan using materials_pkey on materials
Index Cond: ((material_id)::text = 'PI'::text)
(5 rows)
Regarding back-patching, I believe this issue exists before v18, but
it seems that the change in v18 made it common enough to notice,
especially in queries with grouping sets. Given the lack of reports
for versions prior to v18, I'm inclined to back-patch this only to
v18.
Any thoughts?
- Richard
Attachments:
[application/octet-stream] partitioned_table_view_Test_Data_and_Examples.sql.txt (5.9K, ../../eef00885-2b2a-4af2-b0b1-3ba04e93efe5.gehaowu@bitmoe.com/3-partitioned_table_view_Test_Data_and_Examples.sql.txt)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-12 09:28 Richard Guo <guofenglinux@gmail.com>
parent: Haowu Ge <gehaowu@bitmoe.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-12 09:28 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Thu, Dec 11, 2025 at 11:32 AM Haowu Ge <gehaowu@bitmoe.com> wrote:
> Based on the "Minimal Reproducible Example" principle,
> I previously created a simplified version of the example,
> which you successfully fixed. However, during actual testing,
> I found that the behavior does not meet expectations.
> Specifically, I observed inconsistent index behavior when using a view compared to querying the underlying table directly.
This seems to be a separate issue. The query against the view chooses
different indexes than the query against the underlying table. This
isn't the index capability issue you initially reported, but rather a
cost estimation issue.
Upon closer look, the issue is that the planner fails to look through
PHVs when looking up statistical data. Since examine_variable()
relies on strict structural matching, the presence of PHVs prevents
the planner from matching the expression to the table columns. As a
result, the view-based query falls back to default selectivity
estimates, leading to a poor plan choice.
To fix, we can strip all PHVs when looking up statistical data. This
is safe during estimation because PHVs are transparent for the purpose
of statistics lookup; they do not alter the value distribution of the
underlying expression.
Regarding implementation, I considered reusing the stripper for index
operands in 0001 but decided to use a dedicated function. While the
traversal structure is similar, the logic is fundamentally different.
Stripping PHVs for index matching requires strict checks on
phnullingrels and phexpr to ensure execution correctness. For
statistics lookup, however, we can be permissive and unconditionally
strip all PHVs. It could be argued that we can combine them into a
single function using flags, but that seems to unnecessarily entangle
two unrelated logics.
There is one plan change in the regression tests with 0002, but that
is precisely because the row count estimates become more accurate with
this patch. For instance:
on master:
-> Seq Scan on parttbl1 parttbl (cost=0.00..41.88 rows=13 width=8)
(actual time=0.034..0.036 rows=1.00 loops=1)
on patched:
-> Seq Scan on parttbl1 parttbl (cost=0.00..41.88 rows=1 width=8)
(actual time=0.062..0.064 rows=1.00 loops=1)
- Richard
Attachments:
[application/octet-stream] v3-0001-Strip-PlaceHolderVars-from-index-operands.patch (12.7K, ../../CAMbWs4-99AsnnvfTuGj9C_QHXB9aNhjNyhfxXEcOWeAeq=Og5A@mail.gmail.com/2-v3-0001-Strip-PlaceHolderVars-from-index-operands.patch)
download | inline diff:
From 1c5675b3e207b4973401a27d3a7a9d489dde5766 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Nov 2025 16:38:44 +0900
Subject: [PATCH v3 1/2] 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 | 127 ++++++++++++++++++++-
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, 231 insertions(+), 9 deletions(-)
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 5d4f81ee77e..048940a743f 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,
@@ -197,6 +203,8 @@ static Expr *match_clause_to_ordering_op(IndexOptInfo *index,
static bool ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *ec, EquivalenceMember *em,
void *arg);
+static bool contain_strippable_phv_walker(Node *node, void *context);
+static Node *strip_phvs_in_index_operand_mutator(Node *node, void *context);
/*
@@ -4358,12 +4366,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];
@@ -4416,6 +4435,104 @@ match_index_to_operand(Node *operand,
return false;
}
+/*
+ * 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);
+}
+
+/*
+ * 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);
+}
+
/*
* 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 bc417f93840..9ef7f88a3d7 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 9dd65b10254..bbea3af054c 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2897,6 +2897,7 @@ StreamStopReason
String
StringInfo
StringInfoData
+StripPHVContext
StripnullState
SubLink
SubLinkType
--
2.39.5 (Apple Git-154)
[application/octet-stream] v3-0002-Ignore-PlaceHolderVars-when-looking-up-statistics.patch (7.2K, ../../CAMbWs4-99AsnnvfTuGj9C_QHXB9aNhjNyhfxXEcOWeAeq=Og5A@mail.gmail.com/3-v3-0002-Ignore-PlaceHolderVars-when-looking-up-statistics.patch)
download | inline diff:
From 3d1ad16802543c25e627ee81b209a3269ef27647 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 12 Dec 2025 15:49:02 +0900
Subject: [PATCH v3 2/2] Ignore PlaceHolderVars when looking up statistics
When looking up statistical data about an expression, we failed to
look through PlaceHolderVar nodes, treating them as opaque. This
could prevent us from matching an expression to base columns, index
expressions, or extended statistics, as examine_variable() relies on
strict structural matching.
As a result, queries involving PlaceHolderVar nodes often fell back to
default selectivity estimates, potentially leading to poor plan
choices.
This patch updates examine_variable() to strip PlaceHolderVars before
analysis. This is safe during estimation because PlaceHolderVars are
transparent for the purpose of statistics lookup: they do not alter
the value distribution of the underlying expression.
To minimize performance overhead on this hot path, a lightweight
walker first checks for the presence of PlaceHolderVars. The more
expensive mutator is invoked only when necessary.
There is one ensuing plan change in the regression tests, which is
expected and demonstrates the fix: the rowcount estimate becomes much
more accurate with this patch.
---
src/backend/utils/adt/selfuncs.c | 77 +++++++++++++++++++++++++++---
src/test/regress/expected/join.out | 27 ++++++++++-
src/test/regress/sql/join.sql | 10 ++++
3 files changed, 106 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index c760b19db55..ae26b8a407f 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -242,6 +242,9 @@ static char *convert_string_datum(Datum value, Oid typid, Oid collid,
bool *failure);
static double convert_timevalue_to_scalar(Datum value, Oid typid,
bool *failure);
+static Node *strip_all_phvs_deep(Node *node);
+static bool contain_placeholder_walker(Node *node, void *context);
+static Node *strip_all_phvs_mutator(Node *node, void *context);
static void examine_simple_variable(PlannerInfo *root, Var *var,
VariableStatData *vardata);
static void examine_indexcol_variable(PlannerInfo *root, IndexOptInfo *index,
@@ -5632,15 +5635,24 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
/* Save the exposed type of the expression */
vardata->vartype = exprType(node);
- /* Look inside any binary-compatible relabeling */
+ /*
+ * PlaceHolderVars are transparent for the purpose of statistics lookup;
+ * they do not alter the value distribution of the underlying expression.
+ * However, they can obscure the structure, preventing us from recognizing
+ * matches to base columns, index expressions, or extended statistics. So
+ * strip them out first.
+ */
+ basenode = strip_all_phvs_deep(node);
- if (IsA(node, RelabelType))
- basenode = (Node *) ((RelabelType *) node)->arg;
- else
- basenode = node;
+ /*
+ * Look inside any binary-compatible relabeling. We need to handle nested
+ * RelabelType nodes here, because the prior stripping of PlaceHolderVars
+ * may have brought separate RelabelTypes into adjacency.
+ */
+ while (IsA(basenode, RelabelType))
+ basenode = (Node *) ((RelabelType *) basenode)->arg;
/* Fast path for a simple Var */
-
if (IsA(basenode, Var) &&
(varRelid == 0 || varRelid == ((Var *) basenode)->varno))
{
@@ -5934,6 +5946,59 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
bms_free(varnos);
}
+/*
+ * strip_all_phvs_deep
+ * Deeply strip all PlaceHolderVars in an expression.
+
+ * As a performance optimization, we first use a lightweight walker to check
+ * for the presence of any PlaceHolderVars. The expensive mutator is invoked
+ * only if a PlaceHolderVar is found, avoiding unnecessary memory allocation
+ * and tree copying in the common case where no PlaceHolderVars are present.
+ */
+static Node *
+strip_all_phvs_deep(Node *node)
+{
+ if (!contain_placeholder_walker(node, NULL))
+ return node;
+ return strip_all_phvs_mutator(node, NULL);
+}
+
+/*
+ * contain_placeholder_walker
+ * Lightweight walker to check if an expression contains any
+ * PlaceHolderVars
+ */
+static bool
+contain_placeholder_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+ if (IsA(node, PlaceHolderVar))
+ return true;
+
+ return expression_tree_walker(node, contain_placeholder_walker, context);
+}
+
+/*
+ * strip_all_phvs_mutator
+ * Mutator to deeply strip all PlaceHolderVars
+ */
+static Node *
+strip_all_phvs_mutator(Node *node, void *context)
+{
+ if (node == NULL)
+ return NULL;
+ if (IsA(node, PlaceHolderVar))
+ {
+ /* Strip it and recurse into its contained expression */
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+
+ return strip_all_phvs_mutator((Node *) phv->phexpr, context);
+ }
+
+ return expression_tree_mutator(node, strip_all_phvs_mutator, context);
+}
+
/*
* examine_simple_variable
* Handle a simple Var for examine_variable
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index edde9e99893..1416f2943bd 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6877,10 +6877,10 @@ where ss.a = ss.phv and f1 = 0;
QUERY PLAN
------------------------------------
Nested Loop
- -> Seq Scan on int4_tbl
- Filter: (f1 = 0)
-> Seq Scan on parttbl1 parttbl
Filter: (a = 12)
+ -> Seq Scan on int4_tbl
+ Filter: (f1 = 0)
(5 rows)
select * from
@@ -9879,6 +9879,29 @@ GROUP BY s.c1, s.c2;
(7 rows)
DROP TABLE group_tbl;
+-- Test that we ignore PlaceHolderVars when looking up statistics
+EXPLAIN (COSTS OFF)
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+ QUERY PLAN
+--------------------------------------------------
+ Nested Loop
+ -> Seq Scan on tenk1 t2
+ Filter: (unique1 = 42)
+ -> Index Scan using tenk1_unique2 on tenk1 t1
+ Index Cond: (unique2 = t2.unique2)
+ Filter: (unique1 < 100)
+(6 rows)
+
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+ unique1
+---------
+ 42
+(1 row)
+
--
-- Test for a nested loop join involving index scan, transforming OR-clauses
-- to SAOP.
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 7ec84f3b143..b91fb7574df 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -3764,6 +3764,16 @@ GROUP BY s.c1, s.c2;
DROP TABLE group_tbl;
+-- Test that we ignore PlaceHolderVars when looking up statistics
+EXPLAIN (COSTS OFF)
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+
--
-- Test for a nested loop join involving index scan, transforming OR-clauses
-- to SAOP.
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-15 01:23 Haowu Ge <gehaowu@bitmoe.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Haowu Ge @ 2025-12-15 01:23 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Thu, Dec 12, 2025 at 9:28 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> This seems to be a separate issue. The query against the view chooses
> different indexes than the query against the underlying table. This
> isn't the index capability issue you initially reported, but rather a
> cost estimation issue.
Hello Richard,
After testing, your patch can resolve the issue with the sample case I raised earlier.
Thank you.
However, there is another problem (I thought my example was sufficient, but it actually wasn’t).
In my actual scenario, my view includes join conditions,
and the new patch still does not achieve the same index behavior as in PostgreSQL 16.11.
So it might still require your help to look into what the issue is.
Thank you.
--- Regarding the attachment ---
Consistent with the original attachment: demo-v4-0001-partitioned_table_view_Test_Data_and_Examples.sql.txt
New example: demo-v4-0002-View_with_join_Examples.sql.txt
--- EXPLAIN ANALYZE ---
EXPLAIN ANALYZE from pg18.1 (Seq Scan on invoices_2020 and Rows Removed by Filter: 1000)
-> Hash Right Join (cost=260.06..286.43 rows=289 width=20) (actual time=2.040..2.060 rows=116.00 loops=1)
Hash Cond: ((materials2.material_id)::text = (invoices.material_code)::text)
Buffers: shared hit=79
-> Seq Scan on materials2 (cost=0.00..14.90 rows=490 width=58) (actual time=0.012..0.013 rows=0.00 loops=1)
Buffers: shared hit=1
-> Hash (cost=258.59..258.59 rows=118 width=29) (actual time=2.022..2.024 rows=116.00 loops=1)
Buckets: 1024 Batches: 1 Memory Usage: 15kB
Buffers: shared hit=78
-> Append (cost=0.00..258.59 rows=118 width=29) (actual time=1.119..1.973 rows=116.00 loops=1)
Buffers: shared hit=78
-> Seq Scan on invoices_2020 invoices_1 (cost=0.00..43.00 rows=1 width=29) (actual time=0.243..0.243 rows=0.00 loops=1)
Filter: (((sales_org)::text = ANY ('{DF01,DG01}'::text[])) AND ((data_control_scope)::text = 'DD_OG'::text) AND ((valuation_order_flag)::text = '1'::text) AND ((approval_status)::text = 'y'::text) AND (((document_date >= '2024-06-01'::date) AND (document_date <= '2024-12-31'::date)) OR ((document_date >= '2025-01-01'::date) AND (document_date <= '2025-12-09'::date))))
Rows Removed by Filter: 1000
Buffers: shared hit=13
-> Seq Scan on invoices_2021 invoices_2 (cost=0.00..43.00 rows=1 width=29) (actual time=0.255..0.255 rows=0.00 loops=1)
Filter: (((sales_org)::text = ANY ('{DF01,DG01}'::text[])) AND ((data_control_scope)::text = 'DD_OG'::text) AND ((valuation_order_flag)::text = '1'::text) AND ((approval_status)::text = 'y'::text) AND (((document_date >= '2024-06-01'::date) AND (document_date <= '2024-12-31'::date)) OR ((document_date >= '2025-01-01'::date) AND (document_date <= '2025-12-09'::date))))
Rows Removed by Filter: 1000
Buffers: shared hit=13
EXPLAIN ANALYZE from pg16.11 (Recheck Cond: (((document_date >= '202)
-> Hash Right Join (cost=108.69..128.52 rows=309 width=10) (actual time=0.235..0.248 rows=120 loops=1)
Hash Cond: ((materials2.material_id)::text = (invoices.material_code)::text)
-> Seq Scan on materials2 (cost=0.00..14.90 rows=490 width=58) (actual time=0.005..0.005 rows=0 loops=1)
-> Hash (cost=107.11..107.11 rows=126 width=19) (actual time=0.226..0.230 rows=120 loops=1)
Buckets: 1024 Batches: 1 Memory Usage: 15kB
-> Append (cost=8.57..107.11 rows=126 width=19) (actual time=0.039..0.211 rows=120 loops=1)
-> Bitmap Heap Scan on invoices_2020 invoices_1 (cost=8.57..12.60 rows=1 width=19) (actual time=0.007..0.007 rows=0 loops=1)
Recheck Cond: (((document_date >= '2024-06-01'::date) AND (document_date <= '2024-12-31'::date)) OR ((document_date >= '2025-01-01'::date) AND (document_date <= '2025-12-09'::date)))
Filter: (((sales_org)::text = ANY ('{DF01,DG01}'::text[])) AND ((data_control_scope)::text = 'DD_OG'::text) AND ((valuation_order_flag)::text = '1'::text) AND ((approval_status)::text = 'y'::text))
-> BitmapOr (cost=8.57..8.57 rows=1 width=0) (actual time=0.005..0.006 rows=0 loops=1)
-> Bitmap Index Scan on idx_invoices_2020_date (cost=0.00..4.29 rows=1 width=0) (actual time=0.004..0.004 rows=0 loops=1)
Index Cond: ((document_date >= '2024-06-01'::date) AND (document_date <= '2024-12-31'::date))
-> Bitmap Index Scan on idx_invoices_2020_date (cost=0.00..4.29 rows=1 width=0) (actual time=0.001..0.001 rows=0 loops=1)
Index Cond: ((document_date >= '2025-01-01'::date) AND (document_date <= '2025-12-09'::date))
Thanks & Best Regards
_________________________________________________________________________________
Haowu Ge (BG5FRG) | Homepage: https://www.gehaowu.com | PGP:7A06 1F6E DF09 D8A8
Attachments:
[application/octet-stream] demo-Examples-v4-0002-View_with_join_Examples.sql.txt (1.7K, ../../9e659ce0-da20-42ed-ada2-5f585a89677e.gehaowu@bitmoe.com/2-demo-Examples-v4-0002-View_with_join_Examples.sql.txt)
download
[application/octet-stream] demo-v4-0001-partitioned_table_view_Test_Data_and_Examples.sql.txt (5.9K, ../../9e659ce0-da20-42ed-ada2-5f585a89677e.gehaowu@bitmoe.com/3-demo-v4-0001-partitioned_table_view_Test_Data_and_Examples.sql.txt)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-18 08:56 Richard Guo <guofenglinux@gmail.com>
parent: Haowu Ge <gehaowu@bitmoe.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-18 08:56 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Mon, Dec 15, 2025 at 10:23 AM Haowu Ge <gehaowu@bitmoe.com> wrote:
> On Thu, Dec 12, 2025 at 9:28 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > This seems to be a separate issue. The query against the view chooses
> > different indexes than the query against the underlying table. This
> > isn't the index capability issue you initially reported, but rather a
> > cost estimation issue.
> After testing, your patch can resolve the issue with the sample case I raised earlier.
> Thank you.
>
> However, there is another problem (I thought my example was sufficient, but it actually wasn’t).
> In my actual scenario, my view includes join conditions,
> and the new patch still does not achieve the same index behavior as in PostgreSQL 16.11.
This happens because the v3 patch is conservative: it only strips a
PHV if its syntactic scope (phrels) strictly matches the index
relation. In query:
select 1 from (select t1.a as x from t t1, t t2) where x = 1 group by rollup(x);
The syntactic scope of PHV(t1.a) includes both {t1, t2}. As a result,
the check fails when matching against t1's index, which prevents the
PHV from being stripped and the index from being used.
I considered checking PlaceHolderInfo.ph_eval_at instead, which would
be more accurate. However, finding the corresponding PlaceHolderInfo
requires access to root, and passing root into match_index_to_operand
would break ABI compatibility, which we want to avoid.
Upon closer look, it seems that we do not need the syntactic scope
check at all, as long as we ensure that phnullingrels is empty. In
this case, the PHV is either a no-op (from a reduced outer join) or
used for identity separation (grouping sets). In either case, the PHV
is transparent regarding the value, so it should not prevent the
underlying expression from matching the index.
Any thoughts?
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-24 07:50 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-24 07:50 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Thu, Dec 18, 2025 at 5:56 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Upon closer look, it seems that we do not need the syntactic scope
> check at all, as long as we ensure that phnullingrels is empty. In
> this case, the PHV is either a no-op (from a reduced outer join) or
> used for identity separation (grouping sets). In either case, the PHV
> is transparent regarding the value, so it should not prevent the
> underlying expression from matching the index.
Here is the updated version along the lines of this idea.
- Richard
Attachments:
[application/octet-stream] v4-0001-Strip-PlaceHolderVars-from-index-operands.patch (12.6K, ../../CAMbWs4911OFJSybB73rkxfV5L7H+XB1YG-vnAhN1wdQE=4-jYQ@mail.gmail.com/2-v4-0001-Strip-PlaceHolderVars-from-index-operands.patch)
download | inline diff:
From 2d1645d337a0c5cdd61022b37308279337b8f405 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Nov 2025 16:38:44 +0900
Subject: [PATCH v4 1/2] 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.
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 | 109 ++++++++++++++++++++-
src/backend/optimizer/plan/createplan.c | 14 ++-
src/include/optimizer/paths.h | 1 +
src/test/regress/expected/groupingsets.out | 95 ++++++++++++++++++
src/test/regress/sql/groupingsets.sql | 46 +++++++++
5 files changed, 256 insertions(+), 9 deletions(-)
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 5d4f81ee77e..cf2b5f95d13 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -197,6 +197,8 @@ static Expr *match_clause_to_ordering_op(IndexOptInfo *index,
static bool ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *ec, EquivalenceMember *em,
void *arg);
+static bool contain_strippable_phv_walker(Node *node, void *context);
+static Node *strip_phvs_in_index_operand_mutator(Node *node, void *context);
/*
@@ -4358,12 +4360,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.
*/
- if (operand && IsA(operand, RelabelType))
+ operand = strip_phvs_in_index_operand(operand);
+
+ /*
+ * 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.
+ */
+ while (operand && IsA(operand, RelabelType))
operand = (Node *) ((RelabelType *) operand)->arg;
indkey = index->indexkeys[indexcol];
@@ -4416,6 +4429,92 @@ match_index_to_operand(Node *operand,
return false;
}
+/*
+ * 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.
+ *
+ * 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)
+{
+ /* Don't mutate/copy if no target PHVs exist */
+ if (!contain_strippable_phv_walker(operand, NULL))
+ return operand;
+
+ return strip_phvs_in_index_operand_mutator(operand, NULL);
+}
+
+/*
+ * 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.
+ */
+static bool
+contain_strippable_phv_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+
+ if (IsA(node, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+
+ if (bms_is_empty(phv->phnullingrels))
+ 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, replacing it
+ * with its contained expression.
+ */
+static Node *
+strip_phvs_in_index_operand_mutator(Node *node, void *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))
+ {
+ /* 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);
+}
+
/*
* 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 bc417f93840..f1a01cfc544 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);
+
/*
* 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..3ccf158e09d 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);
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..9c812a64b48 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -463,6 +463,101 @@ 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)
+
+explain (costs off)
+select x, y
+ from (select t1.unique1 as x, t1.unique2 as y from tenk1 t1, tenk1 t2) as t
+ where x = 1
+ group by grouping sets (x, y)
+ order by 1, 2;
+ QUERY PLAN
+-------------------------------------------------------------------
+ Sort
+ Sort Key: t1.unique1, t1.unique2
+ -> GroupAggregate
+ Group Key: t1.unique1
+ Sort Key: t1.unique2
+ Group Key: t1.unique2
+ -> Nested Loop
+ -> Index Scan using tenk1_unique1 on tenk1 t1
+ Index Cond: (unique1 = 1)
+ -> Index Only Scan using tenk1_hundred on tenk1 t2
+(10 rows)
+
+select x, y
+ from (select t1.unique1 as x, t1.unique2 as y from tenk1 t1, tenk1 t2) as t
+ where x = 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..c797506c6a0 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -183,6 +183,52 @@ 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;
+
+explain (costs off)
+select x, y
+ from (select t1.unique1 as x, t1.unique2 as y from tenk1 t1, tenk1 t2) as t
+ where x = 1
+ group by grouping sets (x, y)
+ order by 1, 2;
+
+select x, y
+ from (select t1.unique1 as x, t1.unique2 as y from tenk1 t1, tenk1 t2) as t
+ where x = 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 (
--
2.39.5 (Apple Git-154)
[application/octet-stream] v4-0002-Ignore-PlaceHolderVars-when-looking-up-statistics.patch (7.3K, ../../CAMbWs4911OFJSybB73rkxfV5L7H+XB1YG-vnAhN1wdQE=4-jYQ@mail.gmail.com/3-v4-0002-Ignore-PlaceHolderVars-when-looking-up-statistics.patch)
download | inline diff:
From 2328122c237805a2b8b0ec2ab808bcfd1ea6da57 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 12 Dec 2025 15:49:02 +0900
Subject: [PATCH v4 2/2] Ignore PlaceHolderVars when looking up statistics
When looking up statistical data about an expression, we failed to
look through PlaceHolderVar nodes, treating them as opaque. This
could prevent us from matching an expression to base columns, index
expressions, or extended statistics, as examine_variable() relies on
strict structural matching.
As a result, queries involving PlaceHolderVar nodes often fell back to
default selectivity estimates, potentially leading to poor plan
choices.
This patch updates examine_variable() to strip PlaceHolderVars before
analysis. This is safe during estimation because PlaceHolderVars are
transparent for the purpose of statistics lookup: they do not alter
the value distribution of the underlying expression.
To minimize performance overhead on this hot path, a lightweight
walker first checks for the presence of PlaceHolderVars. The more
expensive mutator is invoked only when necessary.
There is one ensuing plan change in the regression tests, which is
expected and demonstrates the fix: the rowcount estimate becomes much
more accurate with this patch.
---
src/backend/utils/adt/selfuncs.c | 81 +++++++++++++++++++++++++++---
src/test/regress/expected/join.out | 27 +++++++++-
src/test/regress/sql/join.sql | 10 ++++
3 files changed, 110 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index c760b19db55..bc3fedd6150 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -242,6 +242,9 @@ static char *convert_string_datum(Datum value, Oid typid, Oid collid,
bool *failure);
static double convert_timevalue_to_scalar(Datum value, Oid typid,
bool *failure);
+static Node *strip_all_phvs_deep(PlannerInfo *root, Node *node);
+static bool contain_placeholder_walker(Node *node, void *context);
+static Node *strip_all_phvs_mutator(Node *node, void *context);
static void examine_simple_variable(PlannerInfo *root, Var *var,
VariableStatData *vardata);
static void examine_indexcol_variable(PlannerInfo *root, IndexOptInfo *index,
@@ -5632,15 +5635,24 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
/* Save the exposed type of the expression */
vardata->vartype = exprType(node);
- /* Look inside any binary-compatible relabeling */
+ /*
+ * PlaceHolderVars are transparent for the purpose of statistics lookup;
+ * they do not alter the value distribution of the underlying expression.
+ * However, they can obscure the structure, preventing us from recognizing
+ * matches to base columns, index expressions, or extended statistics. So
+ * strip them out first.
+ */
+ basenode = strip_all_phvs_deep(root, node);
- if (IsA(node, RelabelType))
- basenode = (Node *) ((RelabelType *) node)->arg;
- else
- basenode = node;
+ /*
+ * Look inside any binary-compatible relabeling. We need to handle nested
+ * RelabelType nodes here, because the prior stripping of PlaceHolderVars
+ * may have brought separate RelabelTypes into adjacency.
+ */
+ while (IsA(basenode, RelabelType))
+ basenode = (Node *) ((RelabelType *) basenode)->arg;
/* Fast path for a simple Var */
-
if (IsA(basenode, Var) &&
(varRelid == 0 || varRelid == ((Var *) basenode)->varno))
{
@@ -5934,6 +5946,63 @@ examine_variable(PlannerInfo *root, Node *node, int varRelid,
bms_free(varnos);
}
+/*
+ * strip_all_phvs_deep
+ * Deeply strip all PlaceHolderVars in an expression.
+
+ * As a performance optimization, we first use a lightweight walker to check
+ * for the presence of any PlaceHolderVars. The expensive mutator is invoked
+ * only if a PlaceHolderVar is found, avoiding unnecessary memory allocation
+ * and tree copying in the common case where no PlaceHolderVars are present.
+ */
+static Node *
+strip_all_phvs_deep(PlannerInfo *root, Node *node)
+{
+ /* If there are no PHVs anywhere, we needn't work hard */
+ if (root->glob->lastPHId == 0)
+ return node;
+
+ if (!contain_placeholder_walker(node, NULL))
+ return node;
+ return strip_all_phvs_mutator(node, NULL);
+}
+
+/*
+ * contain_placeholder_walker
+ * Lightweight walker to check if an expression contains any
+ * PlaceHolderVars
+ */
+static bool
+contain_placeholder_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ return false;
+ if (IsA(node, PlaceHolderVar))
+ return true;
+
+ return expression_tree_walker(node, contain_placeholder_walker, context);
+}
+
+/*
+ * strip_all_phvs_mutator
+ * Mutator to deeply strip all PlaceHolderVars
+ */
+static Node *
+strip_all_phvs_mutator(Node *node, void *context)
+{
+ if (node == NULL)
+ return NULL;
+ if (IsA(node, PlaceHolderVar))
+ {
+ /* Strip it and recurse into its contained expression */
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+
+ return strip_all_phvs_mutator((Node *) phv->phexpr, context);
+ }
+
+ return expression_tree_mutator(node, strip_all_phvs_mutator, context);
+}
+
/*
* examine_simple_variable
* Handle a simple Var for examine_variable
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index edde9e99893..1416f2943bd 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6877,10 +6877,10 @@ where ss.a = ss.phv and f1 = 0;
QUERY PLAN
------------------------------------
Nested Loop
- -> Seq Scan on int4_tbl
- Filter: (f1 = 0)
-> Seq Scan on parttbl1 parttbl
Filter: (a = 12)
+ -> Seq Scan on int4_tbl
+ Filter: (f1 = 0)
(5 rows)
select * from
@@ -9879,6 +9879,29 @@ GROUP BY s.c1, s.c2;
(7 rows)
DROP TABLE group_tbl;
+-- Test that we ignore PlaceHolderVars when looking up statistics
+EXPLAIN (COSTS OFF)
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+ QUERY PLAN
+--------------------------------------------------
+ Nested Loop
+ -> Seq Scan on tenk1 t2
+ Filter: (unique1 = 42)
+ -> Index Scan using tenk1_unique2 on tenk1 t1
+ Index Cond: (unique2 = t2.unique2)
+ Filter: (unique1 < 100)
+(6 rows)
+
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+ unique1
+---------
+ 42
+(1 row)
+
--
-- Test for a nested loop join involving index scan, transforming OR-clauses
-- to SAOP.
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 7ec84f3b143..b91fb7574df 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -3764,6 +3764,16 @@ GROUP BY s.c1, s.c2;
DROP TABLE group_tbl;
+-- Test that we ignore PlaceHolderVars when looking up statistics
+EXPLAIN (COSTS OFF)
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+
+SELECT t1.unique1 FROM tenk1 t1 LEFT JOIN
+ (SELECT *, 42 AS phv FROM tenk1 t2) ss ON t1.unique2 = ss.unique2
+WHERE ss.unique1 = ss.phv AND t1.unique1 < 100;
+
--
-- Test for a nested loop join involving index scan, transforming OR-clauses
-- to SAOP.
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-24 10:05 Haowu Ge <gehaowu@bitmoe.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Haowu Ge @ 2025-12-24 10:05 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On 12/24/25 15:50, Richard Guo wrote:
> On Thu, Dec 18, 2025 at 5:56 PM Richard Guo <guofenglinux@gmail.com> wrote:
>> Upon closer look, it seems that we do not need the syntactic scope
>> check at all, as long as we ensure that phnullingrels is empty. In
>> this case, the PHV is either a no-op (from a reduced outer join) or
>> used for identity separation (grouping sets). In either case, the PHV
>> is transparent regarding the value, so it should not prevent the
>> underlying expression from matching the index.
>
> Here is the updated version along the lines of this idea.
>
> - Richard
Tested and passed in both example and production environments. Thank you
for your work.
--
Thanks & Best Regards
👨💻 Haowu Ge (葛豪武) | 🌐 www.gehaowu.com | 📡 BG5FRG | 🔐 0xDF09D8A8
Attachments:
[application/pgp-signature] OpenPGP_signature.asc (235B, ../../0f894411-0d2c-4455-8403-f342642e49bc@bitmoe.com/2-OpenPGP_signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-28 04:20 Richard Guo <guofenglinux@gmail.com>
parent: Haowu Ge <gehaowu@bitmoe.com>
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guo @ 2025-12-28 04:20 UTC (permalink / raw)
To: Haowu Ge <gehaowu@bitmoe.com>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
I've been back and forth on whether to back-patch these two patches to
v18, given that they can lead to plan changes.
To recap: in v18, we fixed a correctness issue regarding grouping sets
by wrapping Var expressions in PHVs. While necessary, it had the side
effect of making these expressions opaque to the planner: the planner
can no longer match these expressions to indexes or look up their
statistics. As reported here, this causes a severe performance
regression in v18 compared to previous versions.
The proposed fix is to teach the planner to look through these PHVs
when matching indexes or looking up statistics. While this fixes the
regression, it can lead to plan changes, which we want to avoid in
back branches.
However, I'm still inclined to back-patch this to v18 (and no
further), because:
1. The fix only affects queries involving PHVs in indexable or
statistical contexts. The regression tests show that only one
existing query is affected, and in a positive way. So the impact is
pretty scoped.
2. For the affected queries, the current behavior is arguably broken:
the planner is forced to ignore valid indexes and fall back to default
selectivity estimates. I think the fix is an improvement.
3. If we do not back-patch, this severe performance regression will
remain in v18 forever.
Having said that, back-patching a change that affects query plans
still keeps me up at night. Any thoughts?
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-28 16:51 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Tom Lane @ 2025-12-28 16:51 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Haowu Ge <gehaowu@bitmoe.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Richard Guo <guofenglinux@gmail.com> writes:
> I've been back and forth on whether to back-patch these two patches to
> v18, given that they can lead to plan changes.
Yeah, typically we lean to not back-patching planning changes,
for fear of de-optimizing queries that may have been tuned to the
old behavior.
However, in this case there is a mitigating factor: from the users'
point of view v18's behavior is a regression. Affected queries that
relied on indexes were fine in prior versions but now they fail to
use the indexes. v18 is new enough that it seems unlikely that any
applications have been tuned for its behavior; we should worry more
about de-optimizing applications that haven't yet been brought
forward.
So here, I think the don't-break-things rule points towards
back-patching.
regards, tom lane
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized)
@ 2025-12-30 03:47 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 16+ messages in thread
From: Richard Guo @ 2025-12-30 03:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Haowu Ge <gehaowu@bitmoe.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On Mon, Dec 29, 2025 at 1:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So here, I think the don't-break-things rule points towards
> back-patching.
Thanks for the confirmation. I've pushed this patchset and
back-patched it to v18.
Thanks for the report, Haowu. The fix will be in the next minor
release, scheduled for February.
- Richard
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2025-12-30 03:47 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-11-17 08:02 GROUP BY ROLLUP queries on views trigger full table scans (index usage not optimized) Haowu Ge <gehaowu@bitmoe.com>
2025-11-18 08:27 ` Richard Guo <guofenglinux@gmail.com>
2025-11-19 10:07 ` Richard Guo <guofenglinux@gmail.com>
2025-12-08 06:10 ` Richard Guo <guofenglinux@gmail.com>
2025-12-09 06:38 ` Richard Guo <guofenglinux@gmail.com>
2025-12-09 08:54 ` Haowu Ge <gehaowu@bitmoe.com>
2025-12-10 02:01 ` Richard Guo <guofenglinux@gmail.com>
2025-12-11 02:32 ` Haowu Ge <gehaowu@bitmoe.com>
2025-12-12 09:28 ` Richard Guo <guofenglinux@gmail.com>
2025-12-15 01:23 ` Haowu Ge <gehaowu@bitmoe.com>
2025-12-18 08:56 ` Richard Guo <guofenglinux@gmail.com>
2025-12-24 07:50 ` Richard Guo <guofenglinux@gmail.com>
2025-12-24 10:05 ` Haowu Ge <gehaowu@bitmoe.com>
2025-12-28 04:20 ` Richard Guo <guofenglinux@gmail.com>
2025-12-28 16:51 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-12-30 03:47 ` Richard Guo <guofenglinux@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox