public inbox for [email protected]
help / color / mirror / Atom feedRe: Postgres picks suboptimal index after building of an extended statistics
13+ messages / 6 participants
[nested] [flat]
* Re: Postgres picks suboptimal index after building of an extended statistics
@ 2023-11-02 17:37 Tomas Vondra <[email protected]>
2023-11-22 06:31 ` Re: Postgres picks suboptimal index after building of an extended statistics Andrei Lepikhov <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tomas Vondra @ 2023-11-02 17:37 UTC (permalink / raw)
To: Andrey Lepikhov <[email protected]>; Peter Geoghegan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On 9/25/23 06:30, Andrey Lepikhov wrote:
> ...
> I can't stop thinking about this issue. It is bizarre when Postgres
> chooses a non-unique index if a unique index gives us proof of minimum
> scan.
That's true, but no one implemented this heuristics. So the "proof of
minimum scan" is merely hypothetical - the optimizer is unaware of it.
> I don't see a reason to teach the clauselist_selectivity() routine to
> estimate UNIQUE indexes. We add some cycles, but it will work with btree
> indexes only.
I'm not sure I understand what this is meant to say. Can you elaborate?
We only allow UNIQUE for btree indexes anyway, so what exactly is the
problem here?
> Maybe to change compare_path_costs_fuzzily() and add some heuristic, for
> example:
> "If selectivity of both paths gives us no more than 1 row, prefer to use
> a unique index or an index with least selectivity."
>
I don't understand how this would work. What do yo mean by "selectivity
of a path"? AFAICS the problem here is that we estimate a path to return
more rows (while we know there can only be 1, thanks to UNIQUE index).
So how would you know the path does not give us more than 1 row? Surely
you're not proposing compare_path_costs_fuzzily() to do something
expensive like analyzing the paths, or something.
Also, how would it work in upper levels? If you just change which path
we keep, but leave the inaccurate row estimate in place, that may fix
that level, but it's certainly going to confuse later planning steps.
IMHO the problem here is that we produce wrong estimate, so we better
fix that, instead of adding band-aids to other places.
This happens because functional dependencies are very simple type of
statistics - it has some expectations about the input data and also the
queries executed on it. For example it assumes the data is reasonably
homogeneous, so that we can calculate a global "degree".
But the data in the example directly violates that - it has 1000 rows
that are very random (so we'd find no dependencies), and 1000 rows with
perfect dependencies. Hence we end with degree=0.5, which approximates
the dependencies to all data. Not great, true, but that's the price for
simplicity of this statistics kind.
So the simplest solution is to disable dependencies on such data sets.
It's a bit inconvenient/unfortunate we build dependencies by default,
and people may not be aware of there assumptions.
Perhaps we could/should make dependency_degree() more pessimistic when
we find some "violations" of the rule (we intentionally are not strict
about it, because data are imperfect). I don't have a good idea how to
change the formulas, but I'm open to the idea in principle.
The other thing we could do is checking for unique indexes in
clauselist_selectivity, and if we find a match we can just skip the
extended statistics altogether. Not sure how expensive this would be,
but for typical cases (with modest number of indexes) perhaps OK. It
wouldn't work without a unique index, but I don't have a better idea.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Postgres picks suboptimal index after building of an extended statistics
2023-11-02 17:37 Re: Postgres picks suboptimal index after building of an extended statistics Tomas Vondra <[email protected]>
@ 2023-11-22 06:31 ` Andrei Lepikhov <[email protected]>
2023-11-27 04:44 ` Re: Postgres picks suboptimal index after building of an extended statistics Andrei Lepikhov <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Andrei Lepikhov @ 2023-11-22 06:31 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; Peter Geoghegan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Thanks for detaied answer,
On 3/11/2023 00:37, Tomas Vondra wrote:
> On 9/25/23 06:30, Andrey Lepikhov wrote:
>> ...
>> I can't stop thinking about this issue. It is bizarre when Postgres
>> chooses a non-unique index if a unique index gives us proof of minimum
>> scan.
> That's true, but no one implemented this heuristics. So the "proof of
> minimum scan" is merely hypothetical - the optimizer is unaware of it.
See the simple patch in the attachment. There, I have attempted to
resolve situations of uncertainty to avoid making decisions based solely
on the order of indexes in the list.
>> I don't see a reason to teach the clauselist_selectivity() routine to
>> estimate UNIQUE indexes. We add some cycles, but it will work with btree
>> indexes only.
> I'm not sure I understand what this is meant to say. Can you elaborate?
> We only allow UNIQUE for btree indexes anyway, so what exactly is the
> problem here?
Partly, you already answered yourself below: we have unique index
estimation in a few estimation calls, but go through the list of indexes
each time.
Also, for this sake, we would add some input parameters, usually NULL,
because many estimations don't involve indexes at all.
>> Maybe to change compare_path_costs_fuzzily() and add some heuristic, for
>> example:
>> "If selectivity of both paths gives us no more than 1 row, prefer to use
>> a unique index or an index with least selectivity."
> I don't understand how this would work. What do yo mean by "selectivity
> of a path"? AFAICS the problem here is that we estimate a path to return
> more rows (while we know there can only be 1, thanks to UNIQUE index).
Oops, I meant cardinality. See the patch in the attachment.
> So how would you know the path does not give us more than 1 row? Surely
> you're not proposing compare_path_costs_fuzzily() to do something
> expensive like analyzing the paths, or something.
I solely propose to make optimizer more consecutive in its decisions: if
we have one row for both path candidates, use uniqueness of the index or
value of selectivity as one more parameter.
> Also, how would it work in upper levels? If you just change which path
> we keep, but leave the inaccurate row estimate in place, that may fix
> that level, but it's certainly going to confuse later planning steps.
It is designed for the only scan level.
> IMHO the problem here is that we produce wrong estimate, so we better
> fix that, instead of adding band-aids to other places.
Agree. I am looking for a solution to help users somehow resolve such
problems. As an alternative solution, I can propose a selectivity hook
or (maybe even better) - use the pathlist approach and add indexes into
the index list with some predefined order - at first positions, place
unique indexes with more columns, etc.
> This happens because functional dependencies are very simple type of
> statistics - it has some expectations about the input data and also the
> queries executed on it. For example it assumes the data is reasonably
> homogeneous, so that we can calculate a global "degree".
>
> But the data in the example directly violates that - it has 1000 rows
> that are very random (so we'd find no dependencies), and 1000 rows with
> perfect dependencies. Hence we end with degree=0.5, which approximates
> the dependencies to all data. Not great, true, but that's the price for
> simplicity of this statistics kind.
>
> So the simplest solution is to disable dependencies on such data sets.
> It's a bit inconvenient/unfortunate we build dependencies by default,
> and people may not be aware of there assumptions.
>
> Perhaps we could/should make dependency_degree() more pessimistic when
> we find some "violations" of the rule (we intentionally are not strict
> about it, because data are imperfect). I don't have a good idea how to
> change the formulas, but I'm open to the idea in principle.
Thanks for the explanation!
> The other thing we could do is checking for unique indexes in
> clauselist_selectivity, and if we find a match we can just skip the
> extended statistics altogether. Not sure how expensive this would be,
> but for typical cases (with modest number of indexes) perhaps OK. It
> wouldn't work without a unique index, but I don't have a better idea.
It looks a bit expensive for me. But I am ready to try, if current
solution doesn't look applicable.
--
regards,
Andrei Lepikhov
Postgres Professional
From 21677861e101eed22c829e0b14290a56786a12c2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <[email protected]>
Date: Wed, 22 Nov 2023 12:01:39 +0700
Subject: [PATCH] Use an index path with the best selectivity estimation
---
src/backend/optimizer/util/pathnode.c | 40 +++++++++++++++++
.../expected/drop-index-concurrently-1.out | 16 ++++---
src/test/regress/expected/functional_deps.out | 43 +++++++++++++++++++
src/test/regress/expected/join.out | 40 +++++++++--------
src/test/regress/sql/functional_deps.sql | 36 ++++++++++++++++
5 files changed, 149 insertions(+), 26 deletions(-)
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 0b1d17b9d3..32aca83d67 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -454,6 +454,46 @@ add_path(RelOptInfo *parent_rel, Path *new_path)
costcmp = compare_path_costs_fuzzily(new_path, old_path,
STD_FUZZ_FACTOR);
+ /*
+ * Apply some heuristics on index paths.
+ */
+ if (IsA(new_path, IndexPath) && IsA(old_path, IndexPath))
+ {
+ IndexPath *inp = (IndexPath *) new_path;
+ IndexPath *iop = (IndexPath *) old_path;
+
+ if (new_path->rows <= 1.0 && old_path->rows <= 1.0)
+ {
+ if (inp->indexinfo->unique && !iop->indexinfo->unique)
+ /*
+ * When both paths are predicted to produce only one tuple,
+ * the optimiser should prefer choosing a unique index scan
+ * in all cases.
+ */
+ costcmp = COSTS_BETTER1;
+ else if (costcmp != COSTS_DIFFERENT)
+ /*
+ * If the optimiser doesn't have an obviously stable choice
+ * of unique index, increase the chance of avoiding mistakes
+ * by choosing an index with smaller selectivity.
+ * This option makes decision more conservative and looks
+ * debatable.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+ else if (costcmp == COSTS_EQUAL)
+ /*
+ * The optimizer can't differ the value of two index paths.
+ * To avoid making a decision that is based on only an index
+ * order in the list, use some rational strategy based on
+ * selectivity: prefer touching fewer tuples on the disk to
+ * filtering them after.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+
/*
* If the two paths compare differently for startup and total cost,
* then we want to keep both, and we can skip comparing pathkeys and
diff --git a/src/test/isolation/expected/drop-index-concurrently-1.out b/src/test/isolation/expected/drop-index-concurrently-1.out
index 1cb2250891..2392cdb033 100644
--- a/src/test/isolation/expected/drop-index-concurrently-1.out
+++ b/src/test/isolation/expected/drop-index-concurrently-1.out
@@ -12,13 +12,15 @@ step preps: PREPARE getrow_seqscan AS SELECT * FROM test_dc WHERE data = 34 ORDE
step begin: BEGIN;
step disableseq: SET enable_seqscan = false;
step explaini: EXPLAIN (COSTS OFF) EXECUTE getrow_idxscan;
-QUERY PLAN
-----------------------------------------------
-Sort
- Sort Key: id
- -> Index Scan using test_dc_data on test_dc
- Index Cond: (data = 34)
-(4 rows)
+QUERY PLAN
+---------------------------------------------
+Sort
+ Sort Key: id
+ -> Bitmap Heap Scan on test_dc
+ Recheck Cond: (data = 34)
+ -> Bitmap Index Scan on test_dc_data
+ Index Cond: (data = 34)
+(6 rows)
step enableseq: SET enable_seqscan = true;
step explains: EXPLAIN (COSTS OFF) EXECUTE getrow_seqscan;
diff --git a/src/test/regress/expected/functional_deps.out b/src/test/regress/expected/functional_deps.out
index 32381b8ae7..61af99a041 100644
--- a/src/test/regress/expected/functional_deps.out
+++ b/src/test/regress/expected/functional_deps.out
@@ -230,3 +230,46 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
ERROR: column "articles.keywords" must appear in the GROUP BY clause or be used in an aggregate function
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+DROP TABLE t CASCADE;
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..32b33fabd3 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -8629,14 +8629,15 @@ analyze j2;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
- QUERY PLAN
------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
-(5 rows)
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
+ Filter: ((id1 % 1000) = 1)
+(6 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8651,15 +8652,16 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
- QUERY PLAN
-----------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
Index Cond: (id1 = ANY ('{1}'::integer[]))
-(6 rows)
+ Filter: ((id1 % 1000) = 1)
+(7 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8674,12 +8676,12 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 >= any (array[1,5]);
- QUERY PLAN
--------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
-> Index Only Scan using j2_pkey on j2
Index Cond: (id1 >= ANY ('{1,5}'::integer[]))
Filter: ((id1 % 1000) = 1)
diff --git a/src/test/regress/sql/functional_deps.sql b/src/test/regress/sql/functional_deps.sql
index 406490b995..f617ee9269 100644
--- a/src/test/regress/sql/functional_deps.sql
+++ b/src/test/regress/sql/functional_deps.sql
@@ -208,3 +208,39 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
+
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+DROP TABLE t CASCADE;
--
2.43.0
Attachments:
[text/plain] 0001-Use-an-index-path-with-the-best-selectivity-estimati.patch (10.4K, ../../[email protected]/2-0001-Use-an-index-path-with-the-best-selectivity-estimati.patch)
download | inline diff:
From 21677861e101eed22c829e0b14290a56786a12c2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <[email protected]>
Date: Wed, 22 Nov 2023 12:01:39 +0700
Subject: [PATCH] Use an index path with the best selectivity estimation
---
src/backend/optimizer/util/pathnode.c | 40 +++++++++++++++++
.../expected/drop-index-concurrently-1.out | 16 ++++---
src/test/regress/expected/functional_deps.out | 43 +++++++++++++++++++
src/test/regress/expected/join.out | 40 +++++++++--------
src/test/regress/sql/functional_deps.sql | 36 ++++++++++++++++
5 files changed, 149 insertions(+), 26 deletions(-)
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 0b1d17b9d3..32aca83d67 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -454,6 +454,46 @@ add_path(RelOptInfo *parent_rel, Path *new_path)
costcmp = compare_path_costs_fuzzily(new_path, old_path,
STD_FUZZ_FACTOR);
+ /*
+ * Apply some heuristics on index paths.
+ */
+ if (IsA(new_path, IndexPath) && IsA(old_path, IndexPath))
+ {
+ IndexPath *inp = (IndexPath *) new_path;
+ IndexPath *iop = (IndexPath *) old_path;
+
+ if (new_path->rows <= 1.0 && old_path->rows <= 1.0)
+ {
+ if (inp->indexinfo->unique && !iop->indexinfo->unique)
+ /*
+ * When both paths are predicted to produce only one tuple,
+ * the optimiser should prefer choosing a unique index scan
+ * in all cases.
+ */
+ costcmp = COSTS_BETTER1;
+ else if (costcmp != COSTS_DIFFERENT)
+ /*
+ * If the optimiser doesn't have an obviously stable choice
+ * of unique index, increase the chance of avoiding mistakes
+ * by choosing an index with smaller selectivity.
+ * This option makes decision more conservative and looks
+ * debatable.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+ else if (costcmp == COSTS_EQUAL)
+ /*
+ * The optimizer can't differ the value of two index paths.
+ * To avoid making a decision that is based on only an index
+ * order in the list, use some rational strategy based on
+ * selectivity: prefer touching fewer tuples on the disk to
+ * filtering them after.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+
/*
* If the two paths compare differently for startup and total cost,
* then we want to keep both, and we can skip comparing pathkeys and
diff --git a/src/test/isolation/expected/drop-index-concurrently-1.out b/src/test/isolation/expected/drop-index-concurrently-1.out
index 1cb2250891..2392cdb033 100644
--- a/src/test/isolation/expected/drop-index-concurrently-1.out
+++ b/src/test/isolation/expected/drop-index-concurrently-1.out
@@ -12,13 +12,15 @@ step preps: PREPARE getrow_seqscan AS SELECT * FROM test_dc WHERE data = 34 ORDE
step begin: BEGIN;
step disableseq: SET enable_seqscan = false;
step explaini: EXPLAIN (COSTS OFF) EXECUTE getrow_idxscan;
-QUERY PLAN
-----------------------------------------------
-Sort
- Sort Key: id
- -> Index Scan using test_dc_data on test_dc
- Index Cond: (data = 34)
-(4 rows)
+QUERY PLAN
+---------------------------------------------
+Sort
+ Sort Key: id
+ -> Bitmap Heap Scan on test_dc
+ Recheck Cond: (data = 34)
+ -> Bitmap Index Scan on test_dc_data
+ Index Cond: (data = 34)
+(6 rows)
step enableseq: SET enable_seqscan = true;
step explains: EXPLAIN (COSTS OFF) EXECUTE getrow_seqscan;
diff --git a/src/test/regress/expected/functional_deps.out b/src/test/regress/expected/functional_deps.out
index 32381b8ae7..61af99a041 100644
--- a/src/test/regress/expected/functional_deps.out
+++ b/src/test/regress/expected/functional_deps.out
@@ -230,3 +230,46 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
ERROR: column "articles.keywords" must appear in the GROUP BY clause or be used in an aggregate function
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+DROP TABLE t CASCADE;
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..32b33fabd3 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -8629,14 +8629,15 @@ analyze j2;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
- QUERY PLAN
------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
-(5 rows)
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
+ Filter: ((id1 % 1000) = 1)
+(6 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8651,15 +8652,16 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
- QUERY PLAN
-----------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
Index Cond: (id1 = ANY ('{1}'::integer[]))
-(6 rows)
+ Filter: ((id1 % 1000) = 1)
+(7 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8674,12 +8676,12 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 >= any (array[1,5]);
- QUERY PLAN
--------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
-> Index Only Scan using j2_pkey on j2
Index Cond: (id1 >= ANY ('{1,5}'::integer[]))
Filter: ((id1 % 1000) = 1)
diff --git a/src/test/regress/sql/functional_deps.sql b/src/test/regress/sql/functional_deps.sql
index 406490b995..f617ee9269 100644
--- a/src/test/regress/sql/functional_deps.sql
+++ b/src/test/regress/sql/functional_deps.sql
@@ -208,3 +208,39 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
+
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+DROP TABLE t CASCADE;
--
2.43.0
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Postgres picks suboptimal index after building of an extended statistics
2023-11-02 17:37 Re: Postgres picks suboptimal index after building of an extended statistics Tomas Vondra <[email protected]>
2023-11-22 06:31 ` Re: Postgres picks suboptimal index after building of an extended statistics Andrei Lepikhov <[email protected]>
@ 2023-11-27 04:44 ` Andrei Lepikhov <[email protected]>
2023-12-18 13:29 ` Re: Postgres picks suboptimal index after building of an extended statistics Alexander Korotkov <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Andrei Lepikhov @ 2023-11-27 04:44 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Second version of the patch - resolve non-symmetrical decision, thanks
to Teodor Sigaev's review.
--
regards,
Andrei Lepikhov
Postgres Professional
From 604899b6afe70eccbbdbf69ce254f37808c598db Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <[email protected]>
Date: Mon, 27 Nov 2023 11:23:48 +0700
Subject: [PATCH] Choose an index path with the best selectivity estimation.
In the case when optimizer predicts only one row prefer choosing UNIQUE indexes
In other cases, if optimizer treats indexes as equal, make a last attempt
selecting the index with less selectivity - this decision takes away dependency
on the order of indexes in an index list (good for reproduction of some issues)
and proposes one more objective argument to choose specific index.
---
src/backend/optimizer/util/pathnode.c | 42 ++++++++++++++++++
.../expected/drop-index-concurrently-1.out | 16 ++++---
src/test/regress/expected/functional_deps.out | 43 +++++++++++++++++++
src/test/regress/expected/join.out | 40 +++++++++--------
src/test/regress/sql/functional_deps.sql | 36 ++++++++++++++++
5 files changed, 151 insertions(+), 26 deletions(-)
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 0b1d17b9d3..4b5aedd579 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -454,6 +454,48 @@ add_path(RelOptInfo *parent_rel, Path *new_path)
costcmp = compare_path_costs_fuzzily(new_path, old_path,
STD_FUZZ_FACTOR);
+ /*
+ * Apply some heuristics on index paths.
+ */
+ if (IsA(new_path, IndexPath) && IsA(old_path, IndexPath))
+ {
+ IndexPath *inp = (IndexPath *) new_path;
+ IndexPath *iop = (IndexPath *) old_path;
+
+ if (new_path->rows <= 1.0 && old_path->rows <= 1.0)
+ {
+ /*
+ * When both paths are predicted to produce only one tuple,
+ * the optimiser should prefer choosing a unique index scan
+ * in all cases.
+ */
+ if (inp->indexinfo->unique && !iop->indexinfo->unique)
+ costcmp = COSTS_BETTER1;
+ else if (!inp->indexinfo->unique && iop->indexinfo->unique)
+ costcmp = COSTS_BETTER2;
+ else if (costcmp != COSTS_DIFFERENT)
+ /*
+ * If the optimiser doesn't have an obviously stable choice
+ * of unique index, increase the chance of avoiding mistakes
+ * by choosing an index with smaller selectivity.
+ * This option makes decision more conservative and looks
+ * debatable.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+ else if (costcmp == COSTS_EQUAL)
+ /*
+ * The optimizer can't differ the value of two index paths.
+ * To avoid making a decision that is based on only an index
+ * order in the list, use some rational strategy based on
+ * selectivity: prefer touching fewer tuples on the disk to
+ * filtering them after.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+
/*
* If the two paths compare differently for startup and total cost,
* then we want to keep both, and we can skip comparing pathkeys and
diff --git a/src/test/isolation/expected/drop-index-concurrently-1.out b/src/test/isolation/expected/drop-index-concurrently-1.out
index 1cb2250891..2392cdb033 100644
--- a/src/test/isolation/expected/drop-index-concurrently-1.out
+++ b/src/test/isolation/expected/drop-index-concurrently-1.out
@@ -12,13 +12,15 @@ step preps: PREPARE getrow_seqscan AS SELECT * FROM test_dc WHERE data = 34 ORDE
step begin: BEGIN;
step disableseq: SET enable_seqscan = false;
step explaini: EXPLAIN (COSTS OFF) EXECUTE getrow_idxscan;
-QUERY PLAN
-----------------------------------------------
-Sort
- Sort Key: id
- -> Index Scan using test_dc_data on test_dc
- Index Cond: (data = 34)
-(4 rows)
+QUERY PLAN
+---------------------------------------------
+Sort
+ Sort Key: id
+ -> Bitmap Heap Scan on test_dc
+ Recheck Cond: (data = 34)
+ -> Bitmap Index Scan on test_dc_data
+ Index Cond: (data = 34)
+(6 rows)
step enableseq: SET enable_seqscan = true;
step explains: EXPLAIN (COSTS OFF) EXECUTE getrow_seqscan;
diff --git a/src/test/regress/expected/functional_deps.out b/src/test/regress/expected/functional_deps.out
index 32381b8ae7..61af99a041 100644
--- a/src/test/regress/expected/functional_deps.out
+++ b/src/test/regress/expected/functional_deps.out
@@ -230,3 +230,46 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
ERROR: column "articles.keywords" must appear in the GROUP BY clause or be used in an aggregate function
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+DROP TABLE t CASCADE;
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..32b33fabd3 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -8629,14 +8629,15 @@ analyze j2;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
- QUERY PLAN
------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
-(5 rows)
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
+ Filter: ((id1 % 1000) = 1)
+(6 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8651,15 +8652,16 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
- QUERY PLAN
-----------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
Index Cond: (id1 = ANY ('{1}'::integer[]))
-(6 rows)
+ Filter: ((id1 % 1000) = 1)
+(7 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8674,12 +8676,12 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 >= any (array[1,5]);
- QUERY PLAN
--------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
-> Index Only Scan using j2_pkey on j2
Index Cond: (id1 >= ANY ('{1,5}'::integer[]))
Filter: ((id1 % 1000) = 1)
diff --git a/src/test/regress/sql/functional_deps.sql b/src/test/regress/sql/functional_deps.sql
index 406490b995..f617ee9269 100644
--- a/src/test/regress/sql/functional_deps.sql
+++ b/src/test/regress/sql/functional_deps.sql
@@ -208,3 +208,39 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
+
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+DROP TABLE t CASCADE;
--
2.43.0
Attachments:
[text/plain] v2-0001-Choose-an-index-path-with-the-best-selectivity-estim.patch (10.9K, ../../[email protected]/2-v2-0001-Choose-an-index-path-with-the-best-selectivity-estim.patch)
download | inline diff:
From 604899b6afe70eccbbdbf69ce254f37808c598db Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <[email protected]>
Date: Mon, 27 Nov 2023 11:23:48 +0700
Subject: [PATCH] Choose an index path with the best selectivity estimation.
In the case when optimizer predicts only one row prefer choosing UNIQUE indexes
In other cases, if optimizer treats indexes as equal, make a last attempt
selecting the index with less selectivity - this decision takes away dependency
on the order of indexes in an index list (good for reproduction of some issues)
and proposes one more objective argument to choose specific index.
---
src/backend/optimizer/util/pathnode.c | 42 ++++++++++++++++++
.../expected/drop-index-concurrently-1.out | 16 ++++---
src/test/regress/expected/functional_deps.out | 43 +++++++++++++++++++
src/test/regress/expected/join.out | 40 +++++++++--------
src/test/regress/sql/functional_deps.sql | 36 ++++++++++++++++
5 files changed, 151 insertions(+), 26 deletions(-)
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index 0b1d17b9d3..4b5aedd579 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -454,6 +454,48 @@ add_path(RelOptInfo *parent_rel, Path *new_path)
costcmp = compare_path_costs_fuzzily(new_path, old_path,
STD_FUZZ_FACTOR);
+ /*
+ * Apply some heuristics on index paths.
+ */
+ if (IsA(new_path, IndexPath) && IsA(old_path, IndexPath))
+ {
+ IndexPath *inp = (IndexPath *) new_path;
+ IndexPath *iop = (IndexPath *) old_path;
+
+ if (new_path->rows <= 1.0 && old_path->rows <= 1.0)
+ {
+ /*
+ * When both paths are predicted to produce only one tuple,
+ * the optimiser should prefer choosing a unique index scan
+ * in all cases.
+ */
+ if (inp->indexinfo->unique && !iop->indexinfo->unique)
+ costcmp = COSTS_BETTER1;
+ else if (!inp->indexinfo->unique && iop->indexinfo->unique)
+ costcmp = COSTS_BETTER2;
+ else if (costcmp != COSTS_DIFFERENT)
+ /*
+ * If the optimiser doesn't have an obviously stable choice
+ * of unique index, increase the chance of avoiding mistakes
+ * by choosing an index with smaller selectivity.
+ * This option makes decision more conservative and looks
+ * debatable.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+ else if (costcmp == COSTS_EQUAL)
+ /*
+ * The optimizer can't differ the value of two index paths.
+ * To avoid making a decision that is based on only an index
+ * order in the list, use some rational strategy based on
+ * selectivity: prefer touching fewer tuples on the disk to
+ * filtering them after.
+ */
+ costcmp = (inp->indexselectivity < iop->indexselectivity) ?
+ COSTS_BETTER1 : COSTS_BETTER2;
+ }
+
/*
* If the two paths compare differently for startup and total cost,
* then we want to keep both, and we can skip comparing pathkeys and
diff --git a/src/test/isolation/expected/drop-index-concurrently-1.out b/src/test/isolation/expected/drop-index-concurrently-1.out
index 1cb2250891..2392cdb033 100644
--- a/src/test/isolation/expected/drop-index-concurrently-1.out
+++ b/src/test/isolation/expected/drop-index-concurrently-1.out
@@ -12,13 +12,15 @@ step preps: PREPARE getrow_seqscan AS SELECT * FROM test_dc WHERE data = 34 ORDE
step begin: BEGIN;
step disableseq: SET enable_seqscan = false;
step explaini: EXPLAIN (COSTS OFF) EXECUTE getrow_idxscan;
-QUERY PLAN
-----------------------------------------------
-Sort
- Sort Key: id
- -> Index Scan using test_dc_data on test_dc
- Index Cond: (data = 34)
-(4 rows)
+QUERY PLAN
+---------------------------------------------
+Sort
+ Sort Key: id
+ -> Bitmap Heap Scan on test_dc
+ Recheck Cond: (data = 34)
+ -> Bitmap Index Scan on test_dc_data
+ Index Cond: (data = 34)
+(6 rows)
step enableseq: SET enable_seqscan = true;
step explains: EXPLAIN (COSTS OFF) EXECUTE getrow_seqscan;
diff --git a/src/test/regress/expected/functional_deps.out b/src/test/regress/expected/functional_deps.out
index 32381b8ae7..61af99a041 100644
--- a/src/test/regress/expected/functional_deps.out
+++ b/src/test/regress/expected/functional_deps.out
@@ -230,3 +230,46 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
ERROR: column "articles.keywords" must appear in the GROUP BY clause or be used in an aggregate function
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+ QUERY PLAN
+----------------------------------------------------
+ Index Scan using good on t
+ Index Cond: ((c1 = 1) AND (c2 = 1) AND (c4 = 1))
+ Filter: (c3 = 1)
+(3 rows)
+
+DROP TABLE t CASCADE;
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..32b33fabd3 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -8629,14 +8629,15 @@ analyze j2;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
- QUERY PLAN
------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
-(5 rows)
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
+ Filter: ((id1 % 1000) = 1)
+(6 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8651,15 +8652,16 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1;
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
- QUERY PLAN
-----------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
- -> Index Scan using j2_id1_idx on j2
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
+ -> Index Only Scan using j2_pkey on j2
Index Cond: (id1 = ANY ('{1}'::integer[]))
-(6 rows)
+ Filter: ((id1 % 1000) = 1)
+(7 rows)
select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
@@ -8674,12 +8676,12 @@ where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 = any (array[1]);
explain (costs off) select * from j1
inner join j2 on j1.id1 = j2.id1 and j1.id2 = j2.id2
where j1.id1 % 1000 = 1 and j2.id1 % 1000 = 1 and j2.id1 >= any (array[1,5]);
- QUERY PLAN
--------------------------------------------------------
+ QUERY PLAN
+---------------------------------------------------------
Merge Join
- Merge Cond: (j1.id1 = j2.id1)
- Join Filter: (j2.id2 = j1.id2)
- -> Index Scan using j1_id1_idx on j1
+ Merge Cond: ((j1.id1 = j2.id1) AND (j1.id2 = j2.id2))
+ -> Index Only Scan using j1_pkey on j1
+ Filter: ((id1 % 1000) = 1)
-> Index Only Scan using j2_pkey on j2
Index Cond: (id1 >= ANY ('{1,5}'::integer[]))
Filter: ((id1 % 1000) = 1)
diff --git a/src/test/regress/sql/functional_deps.sql b/src/test/regress/sql/functional_deps.sql
index 406490b995..f617ee9269 100644
--- a/src/test/regress/sql/functional_deps.sql
+++ b/src/test/regress/sql/functional_deps.sql
@@ -208,3 +208,39 @@ EXECUTE foo;
ALTER TABLE articles DROP CONSTRAINT articles_pkey RESTRICT;
EXECUTE foo; -- fail
+
+/*
+ * Corner cases of PostgreSQL optimizer:
+ *
+ * Correlations between columns aren't found by DBMS.
+ * Selectivities multiplication of many columns increases total selectivity
+ * error. If such non-true selectivity is so small, that rows estimation
+ * give us absolute minimum (1) then the optimizer can't choose between different
+ * indexes and uses first from the index list (last created).
+ */
+\set scale 100000
+
+CREATE TABLE t AS (
+ SELECT c1 AS c1, -- selectivity(c1)*selectivity(c2)*nrows <= 1
+ c1 AS c2,
+ -- Create two columns with different selectivity.
+ (c1 % 2) AS c3, -- missing from a good index.
+ (c1 % 4) AS c4 -- missing from a bad index.
+ FROM generate_series(1,:scale) AS c1
+);
+UPDATE t SET c1=1,c2=1,c3=1,c4=2 WHERE c1<:scale/1000;
+
+CREATE INDEX bad ON t (c1,c2,c3);
+CREATE INDEX good ON t (c1,c2,c4);
+ANALYZE t; -- update stat on the indexes.
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+-- Set the bad index to the first position in the index list.
+DROP INDEX bad;
+CREATE INDEX bad ON t (c1,c2,c3);
+ANALYZE t;
+
+EXPLAIN (COSTS OFF) SELECT * FROM t WHERE c1=1 AND c2=1 AND c3=1 AND c4=1;
+
+DROP TABLE t CASCADE;
--
2.43.0
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Postgres picks suboptimal index after building of an extended statistics
2023-11-02 17:37 Re: Postgres picks suboptimal index after building of an extended statistics Tomas Vondra <[email protected]>
2023-11-22 06:31 ` Re: Postgres picks suboptimal index after building of an extended statistics Andrei Lepikhov <[email protected]>
2023-11-27 04:44 ` Re: Postgres picks suboptimal index after building of an extended statistics Andrei Lepikhov <[email protected]>
@ 2023-12-18 13:29 ` Alexander Korotkov <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alexander Korotkov @ 2023-12-18 13:29 UTC (permalink / raw)
To: Andrei Lepikhov <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi!
I'd like to get this subject off the ground. The problem originally
described in [1] obviously comes from wrong selectivity estimation.
"Dependencies" extended statistics lead to significant selectivity miss
24/1000 instead of 1/1000. When the estimation is correct, the PostgreSQL
optimizer is capable of choosing the appropriate unique index for the query.
Tom pointed out in [2] that this might be a problem of "Dependencies"
extended statistics. I've rechecked this. The dataset consists of two
parts. The first part defined in the CREATE TABLE statement contains
independent values. The second part defined in the INSERT statement
contains dependent values. "Dependencies" extended statistics estimate
dependency degree as a fraction of rows containing dependent values.
According to this definition, it correctly estimates the dependency degree
as about 0.5 for all the combinations. So, the error in the estimate comes
from the synergy of two factors: MCV estimation of the frequency of
individual values, and spreading of average dependency degree for those
values, which is not relevant for them. I don't think there is a way to
fix "dependencies" extended statistics because it works exactly as
designed. I have to note that instead of fixing "dependencies" extended
statistics you can just add multi-column MCV statistics, which would fix
the problem.
CREATE STATISTICS aestat(dependencies,ndistinct,mcv) ON x,y,z FROM a;
Independently on how well our statistics work, it looks pitiful that we
couldn't fix that using the knowledge of unique constraints on the table.
Despite statistics, which give us just more or less accurate estimates, the
constraint is something we really enforce and thus can rely on. The
patches provided by Andrei in [1], [3], and [4] fix this issue at the index
scan path level. As Thomas pointed out in [5], that could lead to
inconsistency between the number of rows used for unique index scan
estimation and the value displayed in EXPLAIN (and used for other paths).
Even though this was debated in [6], I can confirm this inconsistency.
Thus, with the patch published in [4], I can see the 28-row estimation with
a unique index scan.
` QUERY PLAN
-----------------------------------------------------------------------
Index Only Scan using a_pkey on a (cost=0.28..8.30 rows=28 width=12)
Index Cond: ((x = 1) AND (y = 1) AND (z = 1))
(2 rows)
Also, there is a set of patches [7], [8], and [9], which makes the
optimizer consider path selectivity as long as path costs during the path
selection. I've rechecked that none of these patches could resolve the
original problem described in [1]. Also, I think they are quite tricky.
The model of our optimizer assumes that paths in the list should be the
different ways of getting the same result. If we choose the paths by their
selectivity, that breaks this model. I don't say there is no way for
this. But if we do this, that would require significant rethinking of our
optimizer model and possible revision of a significant part of it. Anyway,
I think if there is still interest in this, that should be moved into a
separate thread to keep this thread focused on the problem described in [1].
Finally, I'd like to note that the issue described in [1] is mostly the
selectivity estimation problem. It could be solved by adding the
multi-column MCV statistics. The patches published so far look more like
hacks for particular use cases rather than appropriate solutions. It still
looks promising to me to use the knowledge of unique constraints during
selectivity estimation [10]. Even though it's hard to implement and
possibly implies some overhead, it fits the current model. I also think
unique contracts could probably be used in some way to improve estimates
even when there is no full match.
Links.
1.
https://www.postgresql.org/message-id/0ca4553c-1f34-12ba-9122-44199d1ced41%40postgrespro.ru
2. https://www.postgresql.org/message-id/3119052.1657231656%40sss.pgh.pa.us
3.
https://www.postgresql.org/message-id/90a1d6ef-c777-b95d-9f77-0065ad4522df%40postgrespro.ru
4.
https://www.postgresql.org/message-id/a5a18d86-c0e5-0ceb-9a18-be1beb2d2944%40postgrespro.ru
5.
https://www.postgresql.org/message-id/f8044836-5d61-a4e0-af82-5821a2a1f0a7%40enterprisedb.com
6.
https://www.postgresql.org/message-id/90a1d6ef-c777-b95d-9f77-0065ad4522df%40postgrespro.ru
7.
https://www.postgresql.org/message-id/2df148b5-0bb8-f80b-ac03-251682fab585%40postgrespro.ru
8.
https://www.postgresql.org/message-id/6fb43191-2df3-4791-b307-be754e648276%40postgrespro.ru
9.
https://www.postgresql.org/message-id/154f786a-06a0-4fb1-b8a4-16c66149731b%40postgrespro.ru
10.
https://www.postgresql.org/message-id/f8044836-5d61-a4e0-af82-5821a2a1f0a7%40enterprisedb.com
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH v2 2/3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 28 +++++----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..1455cdf54a 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ do
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
@@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
- }
+ } while (startPage != endPage);
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--gflnge3qfnp343ni
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v2-0003-Simplify-coding-in-slru.c.patch"
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH v3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 29 +++++++--------------------
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..50bb1d8cfc 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,42 +324,27 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ for (;;)
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
(void) ZeroSUBTRANSPage(startPage);
+ if (startPage == endPage)
+ break;
+
startPage++;
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
}
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--r4q4lfzvecohuykp--
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH v3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 29 +++++++--------------------
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..50bb1d8cfc 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,42 +324,27 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ for (;;)
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
(void) ZeroSUBTRANSPage(startPage);
+ if (startPage == endPage)
+ break;
+
startPage++;
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
}
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--r4q4lfzvecohuykp--
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH v3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 29 +++++++--------------------
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..50bb1d8cfc 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,42 +324,27 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ for (;;)
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
(void) ZeroSUBTRANSPage(startPage);
+ if (startPage == endPage)
+ break;
+
startPage++;
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
}
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--r4q4lfzvecohuykp--
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH v2 2/3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 28 +++++----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..1455cdf54a 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ do
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
@@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
- }
+ } while (startPage != endPage);
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--gflnge3qfnp343ni
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v2-0003-Simplify-coding-in-slru.c.patch"
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH v2 2/3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 28 +++++----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..1455cdf54a 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ do
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
@@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
- }
+ } while (startPage != endPage);
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--gflnge3qfnp343ni
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v2-0003-Simplify-coding-in-slru.c.patch"
^ permalink raw reply [nested|flat] 13+ messages in thread
* [PATCH] Expose checkpoint reason to completion log messages.
@ 2025-12-01 11:18 Soumya S Murali <[email protected]>
2025-12-18 00:19 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Andres Freund <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Soumya S Murali @ 2025-12-01 11:18 UTC (permalink / raw)
To: Michael Banck <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; [email protected]; [email protected]
Hi all,
This patch is an update after reworking the “checkpoint reason” changes as
a standalone patch, separate from the pg_stat_checkpointer additions as
suggested [1]. I applied the patch on a clean tree and verified that the
logging changes work as expected under different workloads. I am attaching
the observations and patch in support for this.This would improve clarity
for performance debugging and help understand checkpoint behavior without
parsing WAL logs manually. Below is one representative checkpoint log entry
after a pgbench run and an explicit CHECKPOINT:
2025-12-01 15:33:30.121 IST [69178] LOG: checkpoint complete (immediate):
wrote 3417 buffers (20.9%), wrote 3 SLRU buffers; 0 WAL file(s) added, 0
removed, 1 recycled; write=0.122 s, sync=0.022 s, total=0.166 s; sync
files=9, longest=0.005 s, average=0.003 s; distance=31304 kB,
estimate=489729 kB; lsn=0/65E92BC8, redo lsn=0/65E92B70
Regarding the pg_stat_checkpointer extensions [1], I understand the
concerns that were raised and I will follow up with a separate full patch
once I incorporate the remaining feedback.
Thank you for the guidance. It has been very helpful. Looking forward to
more further feedback.
Regards,
Soumya
Reference
[1]
https://www.postgresql.org/message-id/flat/CAMtXxw_W6w2Q1QsCvMPnoq3xCMKzH28Zjk_EmL60oP%2BsCTkXOw%40m...
Attachments:
[text/x-patch] 0001-Expose-checkpoint-reason-to-completion-log-messages.patch (3.4K, ../../CAMtXxw9tPwV=NBv5S9GZXMSKPeKv5f9hRhSjZ8__oLsoS5jcuA@mail.gmail.com/3-0001-Expose-checkpoint-reason-to-completion-log-messages.patch)
download | inline diff:
From 4045d7366171a1e512429ac8feb217d8a1199362 Mon Sep 17 00:00:00 2001
From: Soumya <[email protected]>
Date: Mon, 1 Dec 2025 16:17:35 +0530
Subject: [PATCH] Expose checkpoint reason to completion log messages
Signed-off-by: Soumya <[email protected]>
---
src/backend/access/transam/xlog.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 22d0a2e8c3..f699c79c71 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -6750,7 +6750,7 @@ LogCheckpointStart(int flags, bool restartpoint)
* Log end of a checkpoint.
*/
static void
-LogCheckpointEnd(bool restartpoint)
+LogCheckpointEnd(bool restartpoint, int flags)
{
long write_msecs,
sync_msecs,
@@ -6758,6 +6758,16 @@ LogCheckpointEnd(bool restartpoint)
longest_msecs,
average_msecs;
uint64 average_sync_time;
+ const char *ckpt_reason = "timed";
+ /* Determine checkpoint reason */
+ if (flags & CHECKPOINT_IS_SHUTDOWN)
+ ckpt_reason = "shutdown";
+ else if (flags & CHECKPOINT_END_OF_RECOVERY)
+ ckpt_reason = "end-of-recovery";
+ else if (flags & CHECKPOINT_FAST)
+ ckpt_reason = "immediate";
+ else if (flags & CHECKPOINT_FORCE)
+ ckpt_reason = "forced";
CheckpointStats.ckpt_end_t = GetCurrentTimestamp();
@@ -6800,12 +6810,13 @@ LogCheckpointEnd(bool restartpoint)
*/
if (restartpoint)
ereport(LOG,
- (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), "
+ (errmsg("restartpoint complete (%s): wrote %d buffers (%.1f%%), "
"wrote %d SLRU buffers; %d WAL file(s) added, "
"%d removed, %d recycled; write=%ld.%03d s, "
"sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, "
"longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, "
"estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X",
+ ckpt_reason,
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_slru_written,
@@ -6824,12 +6835,13 @@ LogCheckpointEnd(bool restartpoint)
LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo))));
else
ereport(LOG,
- (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), "
+ (errmsg("checkpoint complete (%s): wrote %d buffers (%.1f%%), "
"wrote %d SLRU buffers; %d WAL file(s) added, "
"%d removed, %d recycled; write=%ld.%03d s, "
"sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, "
"longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, "
"estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X",
+ ckpt_reason,
CheckpointStats.ckpt_bufs_written,
(double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers,
CheckpointStats.ckpt_slru_written,
@@ -7418,7 +7430,7 @@ CreateCheckPoint(int flags)
TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning());
/* Real work is done; log and update stats. */
- LogCheckpointEnd(false);
+ LogCheckpointEnd(false, flags);
/* Reset the process title */
update_checkpoint_display(flags, false, true);
@@ -7886,7 +7898,7 @@ CreateRestartPoint(int flags)
TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning());
/* Real work is done; log and update stats. */
- LogCheckpointEnd(true);
+ LogCheckpointEnd(true, flags);
/* Reset the process title */
update_checkpoint_display(flags, true, true);
--
2.34.1
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages.
2025-12-01 11:18 [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]>
@ 2025-12-18 00:19 ` Andres Freund <[email protected]>
2025-12-18 04:46 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Andres Freund @ 2025-12-18 00:19 UTC (permalink / raw)
To: Soumya S Murali <[email protected]>; +Cc: Michael Banck <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected]
Hi,
On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> This patch is an update after reworking the “checkpoint reason” changes as
> a standalone patch, separate from the pg_stat_checkpointer additions as
> suggested [1].
This seems to not pass the tests on any platform:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306
Looks like you need to update 019_replslot_limit.pl for the changed log
format.
I suggest running the tests before submitting.
Greetings,
Andres
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages.
2025-12-01 11:18 [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]>
2025-12-18 00:19 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Andres Freund <[email protected]>
@ 2025-12-18 04:46 ` Soumya S Murali <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Soumya S Murali @ 2025-12-18 04:46 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Banck <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected]
Hi Andres,
On Thu, Dec 18, 2025 at 5:49 AM Andres Freund <[email protected]> wrote:
>
> Hi,
>
> On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote:
> > This patch is an update after reworking the “checkpoint reason” changes as
> > a standalone patch, separate from the pg_stat_checkpointer additions as
> > suggested [1].
>
> This seems to not pass the tests on any platform:
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306
>
> Looks like you need to update 019_replslot_limit.pl for the changed log
> format.
>
> I suggest running the tests before submitting.
>
> Greetings,
>
> Andres
Thank you for checking and for pointing this out. I will fix the
expected log pattern, rerun the test, and will resend an updated patch
once all tests get passed.
Regards,
Soumya
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2025-12-18 04:46 UTC | newest]
Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-11-02 17:37 Re: Postgres picks suboptimal index after building of an extended statistics Tomas Vondra <[email protected]>
2023-11-22 06:31 ` Andrei Lepikhov <[email protected]>
2023-11-27 04:44 ` Andrei Lepikhov <[email protected]>
2023-12-18 13:29 ` Alexander Korotkov <[email protected]>
2024-03-04 10:49 [PATCH v2 2/3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2024-03-04 10:49 [PATCH v3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2024-03-04 10:49 [PATCH v3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2024-03-04 10:49 [PATCH v3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2024-03-04 10:49 [PATCH v2 2/3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2024-03-04 10:49 [PATCH v2 2/3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2025-12-01 11:18 [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]>
2025-12-18 00:19 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Andres Freund <[email protected]>
2025-12-18 04:46 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox