[PATCH] Don't block HOT update by BRIN index
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.
Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1]https://groups.google.com/g/postgresql-cz/c/oxA_v3H17Qg.
It can be viewed online (latest version) on GitHub [2]https://github.com/simi/postgres/pull/7 as well.
- small overview
1. I have added "amhotblocking" flag to index AM descriptor set to
"true" for all, except BRIN, index types. And later in heap_update
method (heapam.c) I do filter attributes based on this new flag,
instead of currently checking for any existing index.
2. I had to enhance the "RelationGetIndexAttrBitmap" function to be
able to return a bitmap of index attribute numbers related to the new
AM flag using "INDEX_ATTR_BITMAP_HOT_BLOCKING" filter.
PS: Originally the "INDEX_ATTR_BITMAP_ALL" filter was used for HOT
check update and most likely could be removed (including all logic
related in RelationGetIndexAttrBitmap), since I have not found any
other usage.
3. I have created an initial regression test using
"pg_stat_get_tuples_hot_updated" to find out HOT was successful on the
BRIN indexed column. Unfortunately "pg_stat_get_tuples_hot_updated" is
not updated immediately and I have not found any way to enforce the
update. Thus (at least for now) I have used a similar approach to
stats.sql using the "wait_for_stats" function (waiting for 30 seconds
and checking each 100ms for change).
I'm attaching patch to CF 2021-07.
[1]: https://groups.google.com/g/postgresql-cz/c/oxA_v3H17Qg
[2]: https://github.com/simi/postgres/pull/7
Attachments:
01-brin-hot.patchtext/x-patch; charset=US-ASCII; name=01-brin-hot.patchDownload
From a997caa436757000970e2794cb44901b9e7cf6d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= <josef.simanek@gmail.com>
Date: Sun, 13 Jun 2021 20:12:40 +0200
Subject: [PATCH] Allow HOT for BRIN indexed columns.
---
src/backend/access/brin/brin.c | 1 +
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/utils/cache/relcache.c | 14 ++++++
src/include/access/amapi.h | 2 +
src/include/utils/rel.h | 1 +
src/include/utils/relcache.h | 3 +-
.../modules/dummy_index_am/dummy_index_am.c | 1 +
src/test/regress/expected/brin.out | 49 +++++++++++++++++++
src/test/regress/sql/brin.sql | 46 +++++++++++++++++
14 files changed, 122 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959a9..f521bb963567 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -108,6 +108,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = false;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index cdd626ff0a44..878a041be073 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -56,6 +56,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = true;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 0683f42c2588..d96ce1c0a99b 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -77,6 +77,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 0752fb38a924..b30b255e021c 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -74,6 +74,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL;
amroutine->amkeytype = INT4OID;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 2433998f39bd..d0ef78b84a44 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3224,7 +3224,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
* Note that we get copies of each bitmap, so we need not worry about
* relcache flush happening midway through.
*/
- hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_ALL);
+ hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_HOT_BLOCKING);
key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
id_attrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_IDENTITY_KEY);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 1360ab80c1e8..99cd99ee861b 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -113,6 +113,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = true;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 9ff280a2526b..c9a037f15819 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -61,6 +61,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 94fbf1aa190c..2220f1389a5b 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5003,6 +5003,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
Bitmapset *uindexattrs; /* columns in unique indexes */
Bitmapset *pkindexattrs; /* columns in the primary index */
Bitmapset *idindexattrs; /* columns in the replica identity */
+ Bitmapset *hotblockingattrs; /* columns with HOT blocking indexes */
List *indexoidlist;
List *newindexoidlist;
Oid relpkindex;
@@ -5023,6 +5024,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
return bms_copy(relation->rd_pkattr);
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return bms_copy(relation->rd_idattr);
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return bms_copy(relation->rd_hotblockingattr);
default:
elog(ERROR, "unknown attrKind %u", attrKind);
}
@@ -5066,6 +5069,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
uindexattrs = NULL;
pkindexattrs = NULL;
idindexattrs = NULL;
+ hotblockingattrs = NULL;
foreach(l, indexoidlist)
{
Oid indexOid = lfirst_oid(l);
@@ -5133,6 +5137,10 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
indexattrs = bms_add_member(indexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
+ if (indexDesc->rd_indam->amhotblocking)
+ hotblockingattrs = bms_add_member(hotblockingattrs,
+ attrnum - FirstLowInvalidHeapAttributeNumber);
+
if (isKey && i < indexDesc->rd_index->indnkeyatts)
uindexattrs = bms_add_member(uindexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
@@ -5179,6 +5187,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
bms_free(uindexattrs);
bms_free(pkindexattrs);
bms_free(idindexattrs);
+ bms_free(hotblockingattrs);
bms_free(indexattrs);
goto restart;
@@ -5193,6 +5202,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
relation->rd_pkattr = NULL;
bms_free(relation->rd_idattr);
relation->rd_idattr = NULL;
+ bms_free(relation->rd_hotblockingattr);
+ relation->rd_hotblockingattr = NULL;
/*
* Now save copies of the bitmaps in the relcache entry. We intentionally
@@ -5205,6 +5216,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
relation->rd_keyattr = bms_copy(uindexattrs);
relation->rd_pkattr = bms_copy(pkindexattrs);
relation->rd_idattr = bms_copy(idindexattrs);
+ relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
relation->rd_indexattr = bms_copy(indexattrs);
MemoryContextSwitchTo(oldcxt);
@@ -5219,6 +5231,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
return pkindexattrs;
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return idindexattrs;
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return hotblockingattrs;
default:
elog(ERROR, "unknown attrKind %u", attrKind);
return NULL;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index d357ebb55980..a0ab70df8938 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -244,6 +244,8 @@ typedef struct IndexAmRoutine
bool amcaninclude;
/* does AM use maintenance_work_mem? */
bool amusemaintenanceworkmem;
+ /* does AM block HOT update? */
+ bool amhotblocking;
/* OR of parallel vacuum flags. See vacuum.h for flags. */
uint8 amparallelvacuumoptions;
/* type of data stored in index, or InvalidOid if variable */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 77d176a93482..d586b6811716 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -159,6 +159,7 @@ typedef struct RelationData
Bitmapset *rd_keyattr; /* cols that can be ref'd by foreign keys */
Bitmapset *rd_pkattr; /* cols included in primary key */
Bitmapset *rd_idattr; /* included in replica identity index */
+ Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */
PublicationActions *rd_pubactions; /* publication actions */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index f772855ac69d..1bc12df96a9c 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -59,7 +59,8 @@ typedef enum IndexAttrBitmapKind
INDEX_ATTR_BITMAP_ALL,
INDEX_ATTR_BITMAP_KEY,
INDEX_ATTR_BITMAP_PRIMARY_KEY,
- INDEX_ATTR_BITMAP_IDENTITY_KEY
+ INDEX_ATTR_BITMAP_IDENTITY_KEY,
+ INDEX_ATTR_BITMAP_HOT_BLOCKING
} IndexAttrBitmapKind;
extern Bitmapset *RelationGetIndexAttrBitmap(Relation relation,
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index 5365b0639ec3..9d409faff540 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -298,6 +298,7 @@ dihandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions = VACUUM_OPTION_NO_PARALLEL;
amroutine->amkeytype = InvalidOid;
diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index e53d6e488567..08dab22b1c64 100644
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -567,3 +567,52 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+SELECT wait_for_hot_stats();
+ wait_for_hot_stats
+--------------------
+
+(1 row)
+
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+ pg_stat_get_tuples_hot_updated
+--------------------------------
+ 1
+(1 row)
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index 3bd866d947a1..740e0b77a548 100644
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -509,3 +509,49 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+
+SELECT wait_for_hot_stats();
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1].
I just found out current patch is breaking partial-index isolation
test. I'm looking into this problem.
Show quoted text
It can be viewed online (latest version) on GitHub [2] as well.
- small overview
1. I have added "amhotblocking" flag to index AM descriptor set to
"true" for all, except BRIN, index types. And later in heap_update
method (heapam.c) I do filter attributes based on this new flag,
instead of currently checking for any existing index.2. I had to enhance the "RelationGetIndexAttrBitmap" function to be
able to return a bitmap of index attribute numbers related to the new
AM flag using "INDEX_ATTR_BITMAP_HOT_BLOCKING" filter.
PS: Originally the "INDEX_ATTR_BITMAP_ALL" filter was used for HOT
check update and most likely could be removed (including all logic
related in RelationGetIndexAttrBitmap), since I have not found any
other usage.3. I have created an initial regression test using
"pg_stat_get_tuples_hot_updated" to find out HOT was successful on the
BRIN indexed column. Unfortunately "pg_stat_get_tuples_hot_updated" is
not updated immediately and I have not found any way to enforce the
update. Thus (at least for now) I have used a similar approach to
stats.sql using the "wait_for_stats" function (waiting for 30 seconds
and checking each 100ms for change).I'm attaching patch to CF 2021-07.
[1] https://groups.google.com/g/postgresql-cz/c/oxA_v3H17Qg
[2] https://github.com/simi/postgres/pull/7
On 6/30/21 12:53 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1].I just found out current patch is breaking partial-index isolation
test. I'm looking into this problem.
The problem is in RelationGetIndexAttrBitmap - the existing code first
walks indnatts, and builds the indexattrs / hotblockingattrs. But then
it also inspects expressions and the predicate (by pull_varattnos), and
the patch fails to do that for hotblockingattrs. Which is why it fails
for partial-index, because that uses an index with a predicate.
So there needs to be something like:
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexExpressions, 1, &hotblockingattrs);
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);
This fixes the failure for me.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
On 6/30/21 12:53 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1].I just found out current patch is breaking partial-index isolation
test. I'm looking into this problem.The problem is in RelationGetIndexAttrBitmap - the existing code first
walks indnatts, and builds the indexattrs / hotblockingattrs. But then
it also inspects expressions and the predicate (by pull_varattnos), and
the patch fails to do that for hotblockingattrs. Which is why it fails
for partial-index, because that uses an index with a predicate.So there needs to be something like:
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexExpressions, 1, &hotblockingattrs);if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);This fixes the failure for me.
Thanks for the hint. I'm attaching a fixed standalone patch.
Show quoted text
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
01-brin-hot-v2.patchtext/x-patch; charset=US-ASCII; name=01-brin-hot-v2.patchDownload
From a997caa436757000970e2794cb44901b9e7cf6d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= <josef.simanek@gmail.com>
Date: Sun, 13 Jun 2021 20:12:40 +0200
Subject: [PATCH 1/2] Allow HOT for BRIN indexed columns.
---
src/backend/access/brin/brin.c | 1 +
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/utils/cache/relcache.c | 14 ++++++
src/include/access/amapi.h | 2 +
src/include/utils/rel.h | 1 +
src/include/utils/relcache.h | 3 +-
.../modules/dummy_index_am/dummy_index_am.c | 1 +
src/test/regress/expected/brin.out | 49 +++++++++++++++++++
src/test/regress/sql/brin.sql | 46 +++++++++++++++++
14 files changed, 122 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959a9..f521bb963567 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -108,6 +108,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = false;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index cdd626ff0a44..878a041be073 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -56,6 +56,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = true;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 0683f42c2588..d96ce1c0a99b 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -77,6 +77,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 0752fb38a924..b30b255e021c 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -74,6 +74,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL;
amroutine->amkeytype = INT4OID;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 2433998f39bd..d0ef78b84a44 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3224,7 +3224,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
* Note that we get copies of each bitmap, so we need not worry about
* relcache flush happening midway through.
*/
- hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_ALL);
+ hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_HOT_BLOCKING);
key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
id_attrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_IDENTITY_KEY);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 1360ab80c1e8..99cd99ee861b 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -113,6 +113,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = true;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 9ff280a2526b..c9a037f15819 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -61,6 +61,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 94fbf1aa190c..2220f1389a5b 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5003,6 +5003,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
Bitmapset *uindexattrs; /* columns in unique indexes */
Bitmapset *pkindexattrs; /* columns in the primary index */
Bitmapset *idindexattrs; /* columns in the replica identity */
+ Bitmapset *hotblockingattrs; /* columns with HOT blocking indexes */
List *indexoidlist;
List *newindexoidlist;
Oid relpkindex;
@@ -5023,6 +5024,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
return bms_copy(relation->rd_pkattr);
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return bms_copy(relation->rd_idattr);
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return bms_copy(relation->rd_hotblockingattr);
default:
elog(ERROR, "unknown attrKind %u", attrKind);
}
@@ -5066,6 +5069,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
uindexattrs = NULL;
pkindexattrs = NULL;
idindexattrs = NULL;
+ hotblockingattrs = NULL;
foreach(l, indexoidlist)
{
Oid indexOid = lfirst_oid(l);
@@ -5133,6 +5137,10 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
indexattrs = bms_add_member(indexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
+ if (indexDesc->rd_indam->amhotblocking)
+ hotblockingattrs = bms_add_member(hotblockingattrs,
+ attrnum - FirstLowInvalidHeapAttributeNumber);
+
if (isKey && i < indexDesc->rd_index->indnkeyatts)
uindexattrs = bms_add_member(uindexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
@@ -5179,6 +5187,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
bms_free(uindexattrs);
bms_free(pkindexattrs);
bms_free(idindexattrs);
+ bms_free(hotblockingattrs);
bms_free(indexattrs);
goto restart;
@@ -5193,6 +5202,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
relation->rd_pkattr = NULL;
bms_free(relation->rd_idattr);
relation->rd_idattr = NULL;
+ bms_free(relation->rd_hotblockingattr);
+ relation->rd_hotblockingattr = NULL;
/*
* Now save copies of the bitmaps in the relcache entry. We intentionally
@@ -5205,6 +5216,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
relation->rd_keyattr = bms_copy(uindexattrs);
relation->rd_pkattr = bms_copy(pkindexattrs);
relation->rd_idattr = bms_copy(idindexattrs);
+ relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
relation->rd_indexattr = bms_copy(indexattrs);
MemoryContextSwitchTo(oldcxt);
@@ -5219,6 +5231,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
return pkindexattrs;
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return idindexattrs;
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return hotblockingattrs;
default:
elog(ERROR, "unknown attrKind %u", attrKind);
return NULL;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index d357ebb55980..a0ab70df8938 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -244,6 +244,8 @@ typedef struct IndexAmRoutine
bool amcaninclude;
/* does AM use maintenance_work_mem? */
bool amusemaintenanceworkmem;
+ /* does AM block HOT update? */
+ bool amhotblocking;
/* OR of parallel vacuum flags. See vacuum.h for flags. */
uint8 amparallelvacuumoptions;
/* type of data stored in index, or InvalidOid if variable */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 77d176a93482..d586b6811716 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -159,6 +159,7 @@ typedef struct RelationData
Bitmapset *rd_keyattr; /* cols that can be ref'd by foreign keys */
Bitmapset *rd_pkattr; /* cols included in primary key */
Bitmapset *rd_idattr; /* included in replica identity index */
+ Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */
PublicationActions *rd_pubactions; /* publication actions */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index f772855ac69d..1bc12df96a9c 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -59,7 +59,8 @@ typedef enum IndexAttrBitmapKind
INDEX_ATTR_BITMAP_ALL,
INDEX_ATTR_BITMAP_KEY,
INDEX_ATTR_BITMAP_PRIMARY_KEY,
- INDEX_ATTR_BITMAP_IDENTITY_KEY
+ INDEX_ATTR_BITMAP_IDENTITY_KEY,
+ INDEX_ATTR_BITMAP_HOT_BLOCKING
} IndexAttrBitmapKind;
extern Bitmapset *RelationGetIndexAttrBitmap(Relation relation,
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index 5365b0639ec3..9d409faff540 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -298,6 +298,7 @@ dihandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions = VACUUM_OPTION_NO_PARALLEL;
amroutine->amkeytype = InvalidOid;
diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index e53d6e488567..08dab22b1c64 100644
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -567,3 +567,52 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+SELECT wait_for_hot_stats();
+ wait_for_hot_stats
+--------------------
+
+(1 row)
+
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+ pg_stat_get_tuples_hot_updated
+--------------------------------
+ 1
+(1 row)
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index 3bd866d947a1..740e0b77a548 100644
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -509,3 +509,49 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+
+SELECT wait_for_hot_stats();
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
From a6167d0c5118d5d054c5ad64595589750f0bf160 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Josef=20=C5=A0im=C3=A1nek?= <josef.simanek@gmail.com>
Date: Wed, 30 Jun 2021 01:41:16 +0200
Subject: [PATCH 2/2] Pull index expressions/predicates to hotblockingattrs.
---
src/backend/utils/cache/relcache.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 2220f1389a5b..585b4a9c073b 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5157,9 +5157,14 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
/* Collect all attributes used in expressions, too */
pull_varattnos(indexExpressions, 1, &indexattrs);
+ if (indexDesc->rd_indam->amhotblocking)
+ pull_varattnos(indexExpressions, 1, &hotblockingattrs);
/* Collect all attributes in the index predicate, too */
pull_varattnos(indexPredicate, 1, &indexattrs);
+ if (indexDesc->rd_indam->amhotblocking)
+ pull_varattnos(indexPredicate, 1, &hotblockingattrs);
+
index_close(indexDesc, AccessShareLock);
}
On 6/30/21 1:43 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:On 6/30/21 12:53 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1].I just found out current patch is breaking partial-index isolation
test. I'm looking into this problem.The problem is in RelationGetIndexAttrBitmap - the existing code first
walks indnatts, and builds the indexattrs / hotblockingattrs. But then
it also inspects expressions and the predicate (by pull_varattnos), and
the patch fails to do that for hotblockingattrs. Which is why it fails
for partial-index, because that uses an index with a predicate.So there needs to be something like:
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexExpressions, 1, &hotblockingattrs);if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);This fixes the failure for me.
Thanks for the hint. I'm attaching a fixed standalone patch.
Thanks, this version seems to be working fine and passes check-world. So
I did another round of review, and all I have are some simple comments:
1) naming stuff (this is very subjective, feel free to disagree)
I wonder if we should rename 'amhotblocking' to 'amblockshot' which
seems more natural to me?
Similarly, maybe rename rd_hotblockingattr to rd_hotattr
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
like
case INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;
I haven't tried, so maybe I'm missing something?
3) The patch should update indexam.sgml with description of the new
field, amhotblocking or how it'll end up named.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2021-Jul-12, Tomas Vondra wrote:
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
... What? I thought the whole point is that BRIN indexes do not cause
the columns to become part of this set, while all other index types do.
If you make them both the same, then there's no point.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
On 7/12/21 10:37 PM, Alvaro Herrera wrote:
On 2021-Jul-12, Tomas Vondra wrote:
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
... What? I thought the whole point is that BRIN indexes do not cause
the columns to become part of this set, while all other index types do.
If you make them both the same, then there's no point.
Well, one of us is confused and it might be me ;-)
The point is that BRIN is the only index type with amhotblocking=false,
so it would return NULL (and thus it does not block HOT). All other
indexes AMs have amblocking=true and so should return rd_indexattr (I
forgot to change that in the code chunk).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
On 6/30/21 1:43 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:On 6/30/21 12:53 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1].I just found out current patch is breaking partial-index isolation
test. I'm looking into this problem.The problem is in RelationGetIndexAttrBitmap - the existing code first
walks indnatts, and builds the indexattrs / hotblockingattrs. But then
it also inspects expressions and the predicate (by pull_varattnos), and
the patch fails to do that for hotblockingattrs. Which is why it fails
for partial-index, because that uses an index with a predicate.So there needs to be something like:
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexExpressions, 1, &hotblockingattrs);if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);This fixes the failure for me.
Thanks for the hint. I'm attaching a fixed standalone patch.
Thanks, this version seems to be working fine and passes check-world. So
I did another round of review, and all I have are some simple comments:1) naming stuff (this is very subjective, feel free to disagree)
I wonder if we should rename 'amhotblocking' to 'amblockshot' which
seems more natural to me?Similarly, maybe rename rd_hotblockingattr to rd_hotattr
OK, I wasn't sure about the naming.
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
relation->rd_indexattr is currently not used (at least I have not
found anything) for anything, except looking if other values are
already loaded.
/* Quick exit if we already computed the result. */
if (relation->rd_indexattr != NULL)
I think it could be replaced with boolean to make it clear other
values (rd_keyattr, rd_pkattr, rd_idattr, rd_hotblockingattr) are
already loaded.
3) The patch should update indexam.sgml with description of the new
field, amhotblocking or how it'll end up named.
I'll do.
Show quoted text
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 7/12/21 10:45 PM, Josef Šimánek wrote:
po 12. 7. 2021 v 22:31 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:On 6/30/21 1:43 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 1:20 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:On 6/30/21 12:53 AM, Josef Šimánek wrote:
st 30. 6. 2021 v 0:31 odesílatel Josef Šimánek <josef.simanek@gmail.com> napsal:
Hello!
Tomáš Vondra has shared a few ideas to improve BRIN index in czech
PostgreSQL mail list some time ago [1 , in czech only]. This is first
try to implement one of those ideas.Currently BRIN index blocks HOT update even it is not linked tuples
directly. I'm attaching the initial patch allowing HOT update even on
BRIN indexed columns. This patch went through an initial review on
czech PostgreSQL mail list [1].I just found out current patch is breaking partial-index isolation
test. I'm looking into this problem.The problem is in RelationGetIndexAttrBitmap - the existing code first
walks indnatts, and builds the indexattrs / hotblockingattrs. But then
it also inspects expressions and the predicate (by pull_varattnos), and
the patch fails to do that for hotblockingattrs. Which is why it fails
for partial-index, because that uses an index with a predicate.So there needs to be something like:
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexExpressions, 1, &hotblockingattrs);if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);This fixes the failure for me.
Thanks for the hint. I'm attaching a fixed standalone patch.
Thanks, this version seems to be working fine and passes check-world. So
I did another round of review, and all I have are some simple comments:1) naming stuff (this is very subjective, feel free to disagree)
I wonder if we should rename 'amhotblocking' to 'amblockshot' which
seems more natural to me?Similarly, maybe rename rd_hotblockingattr to rd_hotattr
OK, I wasn't sure about the naming.
TBH I'm not sure either.
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
relation->rd_indexattr is currently not used (at least I have not
found anything) for anything, except looking if other values are
already loaded./* Quick exit if we already computed the result. */
if (relation->rd_indexattr != NULL)I think it could be replaced with boolean to make it clear other
values (rd_keyattr, rd_pkattr, rd_idattr, rd_hotblockingattr) are
already loaded.
Well, RelationGetIndexAttrBitmap is accessible from extensions, so it
might be used by code passing INDEX_ATTR_BITMAP_ALL. Not sure if there's
such code, of course.
My point is that for amhotblocking=true the bitmaps seem to be exactly
the same, so we can calculate it just once (so replacing it with a bool
flag would not save us anything).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2021-Jul-12, Tomas Vondra wrote:
Well, one of us is confused and it might be me ;-)
:-)
The point is that BRIN is the only index type with amhotblocking=false,
so it would return NULL (and thus it does not block HOT). All other
indexes AMs have amblocking=true and so should return rd_indexattr (I
forgot to change that in the code chunk).
But RelationGetIndexAttrBitmap is called for the table that contains the
index (and probably contains some other indexes too), not for one
specific index. So the bitmap is about the columns involved in *all*
indexes of the table ...
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)
On 7/12/21 10:55 PM, Alvaro Herrera wrote:
On 2021-Jul-12, Tomas Vondra wrote:
Well, one of us is confused and it might be me ;-)
:-)
The point is that BRIN is the only index type with amhotblocking=false,
so it would return NULL (and thus it does not block HOT). All other
indexes AMs have amblocking=true and so should return rd_indexattr (I
forgot to change that in the code chunk).But RelationGetIndexAttrBitmap is called for the table that contains the
index (and probably contains some other indexes too), not for one
specific index. So the bitmap is about the columns involved in *all*
indexes of the table ...
D'oh! Well, I did say I might be confused ...
Yeah, that optimization is not possible, unfortunately.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2021-Jul-12, Josef Šimánek wrote:
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
relation->rd_indexattr is currently not used (at least I have not
found anything) for anything, except looking if other values are
already loaded.
Oh, that's interesting. What this means is that INDEX_ATTR_BITMAP_ALL
is no longer used; its uses must have all been replaced by something
else. It seems the only one that currently exists is for HOT in
heap_update, which this patch replaces with the new one. In a quick
search, no external code depends on it, so I'd be inclined to just
remove it ...
I think a boolean is much simpler. Consider a table with 1600 columns :-)
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
On 7/12/21 11:02 PM, Alvaro Herrera wrote:
On 2021-Jul-12, Josef Šimánek wrote:
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
relation->rd_indexattr is currently not used (at least I have not
found anything) for anything, except looking if other values are
already loaded.Oh, that's interesting. What this means is that INDEX_ATTR_BITMAP_ALL
is no longer used; its uses must have all been replaced by something
else. It seems the only one that currently exists is for HOT in
heap_update, which this patch replaces with the new one. In a quick
search, no external code depends on it, so I'd be inclined to just
remove it ...I think a boolean is much simpler. Consider a table with 1600 columns :-)
I'm not sure how to verify no external code depends on that flag. I have
no idea if there's a plausible use case for it, though.
Even with 1600 columns the amount of wasted memory is only about 200B,
which is not that bad I think. Not great, not terrible.
OTOH most tables won't have any BRIN indexes, in which case indexattr
and hotblockingattr are guaranteed to be exactly the same. So maybe
that's something we could leverage - we need to calculate the "hot"
bitmap, and in most cases we can use it for indexattr too.
Maybe let's leave that for a separate patch, though?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 2021-Jul-12, Tomas Vondra wrote:
I'm not sure how to verify no external code depends on that flag. I have
no idea if there's a plausible use case for it, though.
But we don't *have* to, do we?
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)
po 12. 7. 2021 v 23:15 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
On 7/12/21 11:02 PM, Alvaro Herrera wrote:
On 2021-Jul-12, Josef Šimánek wrote:
2) Do we actually need to calculate and store hotblockingattrs
separately in RelationGetIndexAttrBitmap? It seems to me it's either
NULL (with amhotblocking=false) or equal to indexattrs. So why not to
just get rid of hotblockingattr and rd_hotblockingattr, and do something
likecase INDEX_ATTR_BITMAP_HOT_BLOCKING:
return (amhotblocking) ? bms_copy(rel->rd_hotblockingattr) : NULL;I haven't tried, so maybe I'm missing something?
relation->rd_indexattr is currently not used (at least I have not
found anything) for anything, except looking if other values are
already loaded.Oh, that's interesting. What this means is that INDEX_ATTR_BITMAP_ALL
is no longer used; its uses must have all been replaced by something
else. It seems the only one that currently exists is for HOT in
heap_update, which this patch replaces with the new one. In a quick
search, no external code depends on it, so I'd be inclined to just
remove it ...I think a boolean is much simpler. Consider a table with 1600 columns :-)
I'm not sure how to verify no external code depends on that flag. I have
no idea if there's a plausible use case for it, though.
I tried GitHub search before to ensure at least it is not a widely
used "API". There were no results outside of PostgreSQL code itself in
first 10 pages of results.
Show quoted text
Even with 1600 columns the amount of wasted memory is only about 200B,
which is not that bad I think. Not great, not terrible.OTOH most tables won't have any BRIN indexes, in which case indexattr
and hotblockingattr are guaranteed to be exactly the same. So maybe
that's something we could leverage - we need to calculate the "hot"
bitmap, and in most cases we can use it for indexattr too.Maybe let's leave that for a separate patch, though?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
I took a look at this patch again to see if I can get it polished and
fixed. Per the discussion, I've removed the rd_indexattr list and
replaced it with a simple flag. While doing so, I noticed a couple of
places that should have consider (init or free) rd_hotblockingattr.
Patch 0001 is the v2, 0002 removes the rd_indexattr etc.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0002-fixes-and-remove-rd_indexattr-20211004.patchtext/x-patch; charset=UTF-8; name=0002-fixes-and-remove-rd_indexattr-20211004.patchDownload
From 9728491208b9b2a9f430483b6132930d5766255f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Mon, 4 Oct 2021 16:10:38 +0200
Subject: [PATCH 2/2] fixes and remove rd_indexattr
---
src/backend/utils/cache/relcache.c | 28 +++++++---------------------
src/include/utils/rel.h | 2 +-
src/include/utils/relcache.h | 1 -
3 files changed, 8 insertions(+), 23 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 946e8d95f3..503c9f5bcb 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2377,10 +2377,10 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
list_free_deep(relation->rd_fkeylist);
list_free(relation->rd_indexlist);
list_free(relation->rd_statlist);
- bms_free(relation->rd_indexattr);
bms_free(relation->rd_keyattr);
bms_free(relation->rd_pkattr);
bms_free(relation->rd_idattr);
+ bms_free(relation->rd_hotblockingattr);
if (relation->rd_pubactions)
pfree(relation->rd_pubactions);
if (relation->rd_options)
@@ -5002,7 +5002,6 @@ RelationGetIndexPredicate(Relation relation)
Bitmapset *
RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
{
- Bitmapset *indexattrs; /* indexed columns */
Bitmapset *uindexattrs; /* columns in unique indexes */
Bitmapset *pkindexattrs; /* columns in the primary index */
Bitmapset *idindexattrs; /* columns in the replica identity */
@@ -5015,12 +5014,10 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
MemoryContext oldcxt;
/* Quick exit if we already computed the result. */
- if (relation->rd_indexattr != NULL)
+ if (relation->rd_attrsvalid)
{
switch (attrKind)
{
- case INDEX_ATTR_BITMAP_ALL:
- return bms_copy(relation->rd_indexattr);
case INDEX_ATTR_BITMAP_KEY:
return bms_copy(relation->rd_keyattr);
case INDEX_ATTR_BITMAP_PRIMARY_KEY:
@@ -5059,7 +5056,7 @@ restart:
relreplindex = relation->rd_replidindex;
/*
- * For each index, add referenced attributes to indexattrs.
+ * For each index, add referenced attributes to appropriate bitmaps.
*
* Note: we consider all indexes returned by RelationGetIndexList, even if
* they are not indisready or indisvalid. This is important because an
@@ -5068,7 +5065,6 @@ restart:
* CONCURRENTLY is far enough along that we should ignore the index, it
* won't be returned at all by RelationGetIndexList.
*/
- indexattrs = NULL;
uindexattrs = NULL;
pkindexattrs = NULL;
idindexattrs = NULL;
@@ -5137,9 +5133,6 @@ restart:
*/
if (attrnum != 0)
{
- indexattrs = bms_add_member(indexattrs,
- attrnum - FirstLowInvalidHeapAttributeNumber);
-
if (indexDesc->rd_indam->amhotblocking)
hotblockingattrs = bms_add_member(hotblockingattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
@@ -5159,12 +5152,10 @@ restart:
}
/* Collect all attributes used in expressions, too */
- pull_varattnos(indexExpressions, 1, &indexattrs);
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexExpressions, 1, &hotblockingattrs);
/* Collect all attributes in the index predicate, too */
- pull_varattnos(indexPredicate, 1, &indexattrs);
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);
@@ -5196,14 +5187,11 @@ restart:
bms_free(pkindexattrs);
bms_free(idindexattrs);
bms_free(hotblockingattrs);
- bms_free(indexattrs);
goto restart;
}
/* Don't leak the old values of these bitmaps, if any */
- bms_free(relation->rd_indexattr);
- relation->rd_indexattr = NULL;
bms_free(relation->rd_keyattr);
relation->rd_keyattr = NULL;
bms_free(relation->rd_pkattr);
@@ -5215,8 +5203,8 @@ restart:
/*
* Now save copies of the bitmaps in the relcache entry. We intentionally
- * set rd_indexattr last, because that's the one that signals validity of
- * the values; if we run out of memory before making that copy, we won't
+ * set rd_attrsvalid last, because that's what signals validity of the
+ * values; if we run out of memory before making that copy, we won't
* leave the relcache entry looking like the other ones are valid but
* empty.
*/
@@ -5225,14 +5213,12 @@ restart:
relation->rd_pkattr = bms_copy(pkindexattrs);
relation->rd_idattr = bms_copy(idindexattrs);
relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
- relation->rd_indexattr = bms_copy(indexattrs);
+ relation->rd_attrsvalid = true;
MemoryContextSwitchTo(oldcxt);
/* We return our original working copy for caller to play with */
switch (attrKind)
{
- case INDEX_ATTR_BITMAP_ALL:
- return indexattrs;
case INDEX_ATTR_BITMAP_KEY:
return uindexattrs;
case INDEX_ATTR_BITMAP_PRIMARY_KEY:
@@ -6089,10 +6075,10 @@ load_relcache_init_file(bool shared)
rel->rd_indexlist = NIL;
rel->rd_pkindex = InvalidOid;
rel->rd_replidindex = InvalidOid;
- rel->rd_indexattr = NULL;
rel->rd_keyattr = NULL;
rel->rd_pkattr = NULL;
rel->rd_idattr = NULL;
+ rel->rd_hotblockingattr = NULL;
rel->rd_pubactions = NULL;
rel->rd_statvalid = false;
rel->rd_statlist = NIL;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 1bde51ab25..31281279cf 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -155,7 +155,7 @@ typedef struct RelationData
List *rd_statlist; /* list of OIDs of extended stats */
/* data managed by RelationGetIndexAttrBitmap: */
- Bitmapset *rd_indexattr; /* identifies columns used in indexes */
+ bool rd_attrsvalid; /* are bitmaps of attrs valid? */
Bitmapset *rd_keyattr; /* cols that can be ref'd by foreign keys */
Bitmapset *rd_pkattr; /* cols included in primary key */
Bitmapset *rd_idattr; /* included in replica identity index */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index bf575bedf8..e9f72535cd 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -55,7 +55,6 @@ extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy);
typedef enum IndexAttrBitmapKind
{
- INDEX_ATTR_BITMAP_ALL,
INDEX_ATTR_BITMAP_KEY,
INDEX_ATTR_BITMAP_PRIMARY_KEY,
INDEX_ATTR_BITMAP_IDENTITY_KEY,
--
2.31.1
0001-v2-20211004.patchtext/x-patch; charset=UTF-8; name=0001-v2-20211004.patchDownload
From 29bf6acba8c8ad41dad7406862437ef4fc84cede Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Mon, 4 Oct 2021 15:10:45 +0200
Subject: [PATCH 1/2] v2
---
src/backend/access/brin/brin.c | 1 +
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/utils/cache/relcache.c | 19 +++++++
src/include/access/amapi.h | 2 +
src/include/utils/rel.h | 1 +
src/include/utils/relcache.h | 3 +-
.../modules/dummy_index_am/dummy_index_am.c | 1 +
src/test/regress/expected/brin.out | 49 +++++++++++++++++++
src/test/regress/sql/brin.sql | 46 +++++++++++++++++
14 files changed, 127 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959..f521bb9635 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -108,6 +108,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = false;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 6d2d71be32..066cf3e11a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -56,6 +56,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = true;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 0683f42c25..d96ce1c0a9 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -77,6 +77,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index eb3810494f..81c7da7ec6 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -74,6 +74,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL;
amroutine->amkeytype = INT4OID;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 2da2be1696..c7f332d673 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3224,7 +3224,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
* Note that we get copies of each bitmap, so we need not worry about
* relcache flush happening midway through.
*/
- hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_ALL);
+ hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_HOT_BLOCKING);
key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
id_attrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_IDENTITY_KEY);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 40ad0956e0..bd6d6b1cc9 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -113,6 +113,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = true;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 03a9cd36e6..a2cabb9d81 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -61,6 +61,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 13d9994af3..946e8d95f3 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -5006,6 +5006,7 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
Bitmapset *uindexattrs; /* columns in unique indexes */
Bitmapset *pkindexattrs; /* columns in the primary index */
Bitmapset *idindexattrs; /* columns in the replica identity */
+ Bitmapset *hotblockingattrs; /* columns with HOT blocking indexes */
List *indexoidlist;
List *newindexoidlist;
Oid relpkindex;
@@ -5026,6 +5027,8 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
return bms_copy(relation->rd_pkattr);
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return bms_copy(relation->rd_idattr);
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return bms_copy(relation->rd_hotblockingattr);
default:
elog(ERROR, "unknown attrKind %u", attrKind);
}
@@ -5069,6 +5072,7 @@ restart:
uindexattrs = NULL;
pkindexattrs = NULL;
idindexattrs = NULL;
+ hotblockingattrs = NULL;
foreach(l, indexoidlist)
{
Oid indexOid = lfirst_oid(l);
@@ -5136,6 +5140,10 @@ restart:
indexattrs = bms_add_member(indexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
+ if (indexDesc->rd_indam->amhotblocking)
+ hotblockingattrs = bms_add_member(hotblockingattrs,
+ attrnum - FirstLowInvalidHeapAttributeNumber);
+
if (isKey && i < indexDesc->rd_index->indnkeyatts)
uindexattrs = bms_add_member(uindexattrs,
attrnum - FirstLowInvalidHeapAttributeNumber);
@@ -5152,9 +5160,14 @@ restart:
/* Collect all attributes used in expressions, too */
pull_varattnos(indexExpressions, 1, &indexattrs);
+ if (indexDesc->rd_indam->amhotblocking)
+ pull_varattnos(indexExpressions, 1, &hotblockingattrs);
/* Collect all attributes in the index predicate, too */
pull_varattnos(indexPredicate, 1, &indexattrs);
+ if (indexDesc->rd_indam->amhotblocking)
+ pull_varattnos(indexPredicate, 1, &hotblockingattrs);
+
index_close(indexDesc, AccessShareLock);
}
@@ -5182,6 +5195,7 @@ restart:
bms_free(uindexattrs);
bms_free(pkindexattrs);
bms_free(idindexattrs);
+ bms_free(hotblockingattrs);
bms_free(indexattrs);
goto restart;
@@ -5196,6 +5210,8 @@ restart:
relation->rd_pkattr = NULL;
bms_free(relation->rd_idattr);
relation->rd_idattr = NULL;
+ bms_free(relation->rd_hotblockingattr);
+ relation->rd_hotblockingattr = NULL;
/*
* Now save copies of the bitmaps in the relcache entry. We intentionally
@@ -5208,6 +5224,7 @@ restart:
relation->rd_keyattr = bms_copy(uindexattrs);
relation->rd_pkattr = bms_copy(pkindexattrs);
relation->rd_idattr = bms_copy(idindexattrs);
+ relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
relation->rd_indexattr = bms_copy(indexattrs);
MemoryContextSwitchTo(oldcxt);
@@ -5222,6 +5239,8 @@ restart:
return pkindexattrs;
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return idindexattrs;
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return hotblockingattrs;
default:
elog(ERROR, "unknown attrKind %u", attrKind);
return NULL;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index d357ebb559..a0ab70df89 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -244,6 +244,8 @@ typedef struct IndexAmRoutine
bool amcaninclude;
/* does AM use maintenance_work_mem? */
bool amusemaintenanceworkmem;
+ /* does AM block HOT update? */
+ bool amhotblocking;
/* OR of parallel vacuum flags. See vacuum.h for flags. */
uint8 amparallelvacuumoptions;
/* type of data stored in index, or InvalidOid if variable */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b4faa1c123..1bde51ab25 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -159,6 +159,7 @@ typedef struct RelationData
Bitmapset *rd_keyattr; /* cols that can be ref'd by foreign keys */
Bitmapset *rd_pkattr; /* cols included in primary key */
Bitmapset *rd_idattr; /* included in replica identity index */
+ Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */
PublicationActions *rd_pubactions; /* publication actions */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index d2c17575f6..bf575bedf8 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -58,7 +58,8 @@ typedef enum IndexAttrBitmapKind
INDEX_ATTR_BITMAP_ALL,
INDEX_ATTR_BITMAP_KEY,
INDEX_ATTR_BITMAP_PRIMARY_KEY,
- INDEX_ATTR_BITMAP_IDENTITY_KEY
+ INDEX_ATTR_BITMAP_IDENTITY_KEY,
+ INDEX_ATTR_BITMAP_HOT_BLOCKING
} IndexAttrBitmapKind;
extern Bitmapset *RelationGetIndexAttrBitmap(Relation relation,
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index 5365b0639e..9d409faff5 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -298,6 +298,7 @@ dihandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions = VACUUM_OPTION_NO_PARALLEL;
amroutine->amkeytype = InvalidOid;
diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index e53d6e4885..08dab22b1c 100644
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -567,3 +567,52 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+SELECT wait_for_hot_stats();
+ wait_for_hot_stats
+--------------------
+
+(1 row)
+
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+ pg_stat_get_tuples_hot_updated
+--------------------------------
+ 1
+(1 row)
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index 3bd866d947..740e0b77a5 100644
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -509,3 +509,49 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+
+SELECT wait_for_hot_stats();
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
--
2.31.1
po 4. 10. 2021 v 16:17 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
Hi,
I took a look at this patch again to see if I can get it polished and
fixed. Per the discussion, I've removed the rd_indexattr list and
replaced it with a simple flag. While doing so, I noticed a couple of
places that should have consider (init or free) rd_hotblockingattr.
Thanks for finishing this. I can confirm both patches do apply without
problems. I did some simple testing locally and everything worked as
intended.
Show quoted text
Patch 0001 is the v2, 0002 removes the rd_indexattr etc.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
I've polished the patch a bit, with the goal to get it committed. I've
added the new amhotblocking flag to indexam.sgml (and I'm wondering if
there's another place in docs for more details).
But then I realized there's an issue in handling the predicate. Consider
this example:
drop table if exists t;
create table t (a int, b int);
insert into t values (1, 100);
create index on t using brin (b) where a = 2;
update t set a = 2;
explain analyze select * from t where a = 2 and b = 100;
set enable_seqscan = off;
explain analyze select * from t where a = 2 and b = 100;
With the previous version of the patch, the explains are this:
QUERY PLAN
----------------------------------------------------------------------
Seq Scan on t (cost=0.00..1.01 rows=1 width=8)
(actual time=0.006..0.007 rows=1 loops=1)
Filter: ((a = 2) AND (b = 100))
Planning Time: 0.040 ms
Execution Time: 0.018 ms
(4 rows)
QUERY PLAN
----------------------------------------------------------------------
Bitmap Heap Scan on t (cost=12.03..16.05 rows=1 width=8)
(actual time=0.007..0.009 rows=0 loops=1)
Recheck Cond: ((b = 100) AND (a = 2))
-> Bitmap Index Scan on t_b_idx (cost=0.00..12.03 rows=1 width=0)
(actual time=0.006..0.006 rows=0 loops=1)
Index Cond: (b = 100)
Planning Time: 0.041 ms
Execution Time: 0.026 ms
(6 rows)
Notice that the second plan (using the brin index) produces 0 rows,
which is obviously wrong. Clearly, the index was not updated.
I think this is caused by simple thinko in RelationGetIndexAttrBitmap,
which did this:
/* Collect all attributes in the index predicate, too */
if (indexDesc->rd_indam->amhotblocking)
pull_varattnos(indexPredicate, 1, &hotblockingattrs);
I think this is wrong - we should not ignore the predicate based on
amhotblocking, because then we'll fail to notice an update making the
tuple match the index predicate (as in the example).
The way I understand heap_update() it does not try to determine if the
update makes the tuple indexable, it just disables HOT when it might
happen. The attached patch just calls pull_varattnos every time.
I wonder if we might be a bit smarter about the predicates vs. HOT, and
disable HOT only when the tuple becomes indexable after the update.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0001-Ignore-BRIN-indexes-when-checking-for-HOT-u-20211105.patchtext/x-patch; charset=UTF-8; name=0001-Ignore-BRIN-indexes-when-checking-for-HOT-u-20211105.patchDownload
From eb47a390cc65d16cd29cf3605448c66943e63a5f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
Date: Mon, 4 Oct 2021 15:10:45 +0200
Subject: [PATCH] Ignore BRIN indexes when checking for HOT udpates
When determining whether HOT is possible or an index needs to be
updated, we can ignore attributes indexed only by BRIN indexes. There
are no pointers to individual tuples in BRIN, and the page range summary
will be updated automatically.
This also removes rd_indexattr list, and replaces it with rd_attrsvalid
flag. The list was not used anywhere, and a simple flag is sufficient.
Patch by Josef Simanek, various fixes and improvements by me.
Author: Josef Simanek
Reviewed-by: Tomas Vondra, Alvaro Herrera
Discussion: https://postgr.es/m/CAFp7QwpMRGcDAQumN7onN9HjrJ3u4X3ZRXdGFT0K5G2JWvnbWg%40mail.gmail.com
---
doc/src/sgml/indexam.sgml | 2 +
src/backend/access/brin/brin.c | 1 +
src/backend/access/gin/ginutil.c | 1 +
src/backend/access/gist/gist.c | 1 +
src/backend/access/hash/hash.c | 1 +
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/nbtree/nbtree.c | 1 +
src/backend/access/spgist/spgutils.c | 1 +
src/backend/utils/cache/relcache.c | 44 +++++++++--------
src/include/access/amapi.h | 2 +
src/include/utils/rel.h | 3 +-
src/include/utils/relcache.h | 4 +-
.../modules/dummy_index_am/dummy_index_am.c | 1 +
src/test/regress/expected/brin.out | 49 +++++++++++++++++++
src/test/regress/sql/brin.sql | 46 +++++++++++++++++
15 files changed, 135 insertions(+), 24 deletions(-)
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index cf359fa9ff..129e2ebd3e 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -126,6 +126,8 @@ typedef struct IndexAmRoutine
bool amcaninclude;
/* does AM use maintenance_work_mem? */
bool amusemaintenanceworkmem;
+ /* does AM block HOT update? */
+ bool amhotblocking;
/* OR of parallel vacuum flags */
uint8 amparallelvacuumoptions;
/* type of data stored in index, or InvalidOid if variable */
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index ccc9fa0959..f521bb9635 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -108,6 +108,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = false;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 6d2d71be32..066cf3e11a 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -56,6 +56,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = true;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 0683f42c25..d96ce1c0a9 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -77,6 +77,7 @@ gisthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index eb3810494f..81c7da7ec6 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -74,6 +74,7 @@ hashhandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL;
amroutine->amkeytype = INT4OID;
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ec234a5e59..6496bf92a2 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3224,7 +3224,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
* Note that we get copies of each bitmap, so we need not worry about
* relcache flush happening midway through.
*/
- hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_ALL);
+ hot_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_HOT_BLOCKING);
key_attrs = RelationGetIndexAttrBitmap(relation, INDEX_ATTR_BITMAP_KEY);
id_attrs = RelationGetIndexAttrBitmap(relation,
INDEX_ATTR_BITMAP_IDENTITY_KEY);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 40ad0956e0..bd6d6b1cc9 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -113,6 +113,7 @@ bthandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = true;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 03a9cd36e6..a2cabb9d81 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -61,6 +61,7 @@ spghandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = true;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions =
VACUUM_OPTION_PARALLEL_BULKDEL | VACUUM_OPTION_PARALLEL_COND_CLEANUP;
amroutine->amkeytype = InvalidOid;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 9fa9e671a1..466e55884a 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2428,10 +2428,10 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
list_free_deep(relation->rd_fkeylist);
list_free(relation->rd_indexlist);
list_free(relation->rd_statlist);
- bms_free(relation->rd_indexattr);
bms_free(relation->rd_keyattr);
bms_free(relation->rd_pkattr);
bms_free(relation->rd_idattr);
+ bms_free(relation->rd_hotblockingattr);
if (relation->rd_pubactions)
pfree(relation->rd_pubactions);
if (relation->rd_options)
@@ -5105,10 +5105,10 @@ RelationGetIndexPredicate(Relation relation)
Bitmapset *
RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
{
- Bitmapset *indexattrs; /* indexed columns */
Bitmapset *uindexattrs; /* columns in unique indexes */
Bitmapset *pkindexattrs; /* columns in the primary index */
Bitmapset *idindexattrs; /* columns in the replica identity */
+ Bitmapset *hotblockingattrs; /* columns with HOT blocking indexes */
List *indexoidlist;
List *newindexoidlist;
Oid relpkindex;
@@ -5117,18 +5117,18 @@ RelationGetIndexAttrBitmap(Relation relation, IndexAttrBitmapKind attrKind)
MemoryContext oldcxt;
/* Quick exit if we already computed the result. */
- if (relation->rd_indexattr != NULL)
+ if (relation->rd_attrsvalid)
{
switch (attrKind)
{
- case INDEX_ATTR_BITMAP_ALL:
- return bms_copy(relation->rd_indexattr);
case INDEX_ATTR_BITMAP_KEY:
return bms_copy(relation->rd_keyattr);
case INDEX_ATTR_BITMAP_PRIMARY_KEY:
return bms_copy(relation->rd_pkattr);
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return bms_copy(relation->rd_idattr);
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return bms_copy(relation->rd_hotblockingattr);
default:
elog(ERROR, "unknown attrKind %u", attrKind);
}
@@ -5159,7 +5159,7 @@ restart:
relreplindex = relation->rd_replidindex;
/*
- * For each index, add referenced attributes to indexattrs.
+ * For each index, add referenced attributes to appropriate bitmaps.
*
* Note: we consider all indexes returned by RelationGetIndexList, even if
* they are not indisready or indisvalid. This is important because an
@@ -5168,10 +5168,10 @@ restart:
* CONCURRENTLY is far enough along that we should ignore the index, it
* won't be returned at all by RelationGetIndexList.
*/
- indexattrs = NULL;
uindexattrs = NULL;
pkindexattrs = NULL;
idindexattrs = NULL;
+ hotblockingattrs = NULL;
foreach(l, indexoidlist)
{
Oid indexOid = lfirst_oid(l);
@@ -5236,8 +5236,9 @@ restart:
*/
if (attrnum != 0)
{
- indexattrs = bms_add_member(indexattrs,
- attrnum - FirstLowInvalidHeapAttributeNumber);
+ if (indexDesc->rd_indam->amhotblocking)
+ hotblockingattrs = bms_add_member(hotblockingattrs,
+ attrnum - FirstLowInvalidHeapAttributeNumber);
if (isKey && i < indexDesc->rd_index->indnkeyatts)
uindexattrs = bms_add_member(uindexattrs,
@@ -5254,10 +5255,11 @@ restart:
}
/* Collect all attributes used in expressions, too */
- pull_varattnos(indexExpressions, 1, &indexattrs);
+ if (indexDesc->rd_indam->amhotblocking)
+ pull_varattnos(indexExpressions, 1, &hotblockingattrs);
/* Collect all attributes in the index predicate, too */
- pull_varattnos(indexPredicate, 1, &indexattrs);
+ pull_varattnos(indexPredicate, 1, &hotblockingattrs);
index_close(indexDesc, AccessShareLock);
}
@@ -5285,25 +5287,25 @@ restart:
bms_free(uindexattrs);
bms_free(pkindexattrs);
bms_free(idindexattrs);
- bms_free(indexattrs);
+ bms_free(hotblockingattrs);
goto restart;
}
/* Don't leak the old values of these bitmaps, if any */
- bms_free(relation->rd_indexattr);
- relation->rd_indexattr = NULL;
bms_free(relation->rd_keyattr);
relation->rd_keyattr = NULL;
bms_free(relation->rd_pkattr);
relation->rd_pkattr = NULL;
bms_free(relation->rd_idattr);
relation->rd_idattr = NULL;
+ bms_free(relation->rd_hotblockingattr);
+ relation->rd_hotblockingattr = NULL;
/*
* Now save copies of the bitmaps in the relcache entry. We intentionally
- * set rd_indexattr last, because that's the one that signals validity of
- * the values; if we run out of memory before making that copy, we won't
+ * set rd_attrsvalid last, because that's what signals validity of the
+ * values; if we run out of memory before making that copy, we won't
* leave the relcache entry looking like the other ones are valid but
* empty.
*/
@@ -5311,20 +5313,21 @@ restart:
relation->rd_keyattr = bms_copy(uindexattrs);
relation->rd_pkattr = bms_copy(pkindexattrs);
relation->rd_idattr = bms_copy(idindexattrs);
- relation->rd_indexattr = bms_copy(indexattrs);
+ relation->rd_hotblockingattr = bms_copy(hotblockingattrs);
+ relation->rd_attrsvalid = true;
MemoryContextSwitchTo(oldcxt);
/* We return our original working copy for caller to play with */
switch (attrKind)
{
- case INDEX_ATTR_BITMAP_ALL:
- return indexattrs;
case INDEX_ATTR_BITMAP_KEY:
return uindexattrs;
case INDEX_ATTR_BITMAP_PRIMARY_KEY:
return pkindexattrs;
case INDEX_ATTR_BITMAP_IDENTITY_KEY:
return idindexattrs;
+ case INDEX_ATTR_BITMAP_HOT_BLOCKING:
+ return hotblockingattrs;
default:
elog(ERROR, "unknown attrKind %u", attrKind);
return NULL;
@@ -6180,10 +6183,11 @@ load_relcache_init_file(bool shared)
rel->rd_indexlist = NIL;
rel->rd_pkindex = InvalidOid;
rel->rd_replidindex = InvalidOid;
- rel->rd_indexattr = NULL;
+ rel->rd_attrsvalid = false;
rel->rd_keyattr = NULL;
rel->rd_pkattr = NULL;
rel->rd_idattr = NULL;
+ rel->rd_hotblockingattr = NULL;
rel->rd_pubactions = NULL;
rel->rd_statvalid = false;
rel->rd_statlist = NIL;
diff --git a/src/include/access/amapi.h b/src/include/access/amapi.h
index d357ebb559..a0ab70df89 100644
--- a/src/include/access/amapi.h
+++ b/src/include/access/amapi.h
@@ -244,6 +244,8 @@ typedef struct IndexAmRoutine
bool amcaninclude;
/* does AM use maintenance_work_mem? */
bool amusemaintenanceworkmem;
+ /* does AM block HOT update? */
+ bool amhotblocking;
/* OR of parallel vacuum flags. See vacuum.h for flags. */
uint8 amparallelvacuumoptions;
/* type of data stored in index, or InvalidOid if variable */
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index b4faa1c123..31281279cf 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -155,10 +155,11 @@ typedef struct RelationData
List *rd_statlist; /* list of OIDs of extended stats */
/* data managed by RelationGetIndexAttrBitmap: */
- Bitmapset *rd_indexattr; /* identifies columns used in indexes */
+ bool rd_attrsvalid; /* are bitmaps of attrs valid? */
Bitmapset *rd_keyattr; /* cols that can be ref'd by foreign keys */
Bitmapset *rd_pkattr; /* cols included in primary key */
Bitmapset *rd_idattr; /* included in replica identity index */
+ Bitmapset *rd_hotblockingattr; /* cols blocking HOT update */
PublicationActions *rd_pubactions; /* publication actions */
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index aa060ef115..82316bba54 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -55,10 +55,10 @@ extern bytea **RelationGetIndexAttOptions(Relation relation, bool copy);
typedef enum IndexAttrBitmapKind
{
- INDEX_ATTR_BITMAP_ALL,
INDEX_ATTR_BITMAP_KEY,
INDEX_ATTR_BITMAP_PRIMARY_KEY,
- INDEX_ATTR_BITMAP_IDENTITY_KEY
+ INDEX_ATTR_BITMAP_IDENTITY_KEY,
+ INDEX_ATTR_BITMAP_HOT_BLOCKING
} IndexAttrBitmapKind;
extern Bitmapset *RelationGetIndexAttrBitmap(Relation relation,
diff --git a/src/test/modules/dummy_index_am/dummy_index_am.c b/src/test/modules/dummy_index_am/dummy_index_am.c
index 5365b0639e..9d409faff5 100644
--- a/src/test/modules/dummy_index_am/dummy_index_am.c
+++ b/src/test/modules/dummy_index_am/dummy_index_am.c
@@ -298,6 +298,7 @@ dihandler(PG_FUNCTION_ARGS)
amroutine->amcanparallel = false;
amroutine->amcaninclude = false;
amroutine->amusemaintenanceworkmem = false;
+ amroutine->amhotblocking = true;
amroutine->amparallelvacuumoptions = VACUUM_OPTION_NO_PARALLEL;
amroutine->amkeytype = InvalidOid;
diff --git a/src/test/regress/expected/brin.out b/src/test/regress/expected/brin.out
index e53d6e4885..08dab22b1c 100644
--- a/src/test/regress/expected/brin.out
+++ b/src/test/regress/expected/brin.out
@@ -567,3 +567,52 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+SELECT wait_for_hot_stats();
+ wait_for_hot_stats
+--------------------
+
+(1 row)
+
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+ pg_stat_get_tuples_hot_updated
+--------------------------------
+ 1
+(1 row)
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
diff --git a/src/test/regress/sql/brin.sql b/src/test/regress/sql/brin.sql
index 3bd866d947..740e0b77a5 100644
--- a/src/test/regress/sql/brin.sql
+++ b/src/test/regress/sql/brin.sql
@@ -509,3 +509,49 @@ SELECT * FROM brintest_3 WHERE b < '0';
DROP TABLE brintest_3;
RESET enable_seqscan;
+
+-- test BRIN index doesn't block HOT update
+CREATE TABLE brin_hot (
+ id integer PRIMARY KEY,
+ val integer NOT NULL
+) WITH (autovacuum_enabled = off, fillfactor = 70);
+
+INSERT INTO brin_hot SELECT *, 0 FROM generate_series(1, 235);
+CREATE INDEX val_brin ON brin_hot using brin(val);
+
+CREATE FUNCTION wait_for_hot_stats() RETURNS void AS $$
+DECLARE
+ start_time timestamptz := clock_timestamp();
+ updated bool;
+BEGIN
+ -- we don't want to wait forever; loop will exit after 30 seconds
+ FOR i IN 1 .. 300 LOOP
+ SELECT (pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid) > 0) INTO updated;
+ EXIT WHEN updated;
+
+ -- wait a little
+ PERFORM pg_sleep_for('100 milliseconds');
+ -- reset stats snapshot so we can test again
+ PERFORM pg_stat_clear_snapshot();
+ END LOOP;
+ -- report time waited in postmaster log (where it won't change test output)
+ RAISE log 'wait_for_hot_stats delayed % seconds',
+ EXTRACT(epoch FROM clock_timestamp() - start_time);
+END
+$$ LANGUAGE plpgsql;
+
+UPDATE brin_hot SET val = -3 WHERE id = 42;
+
+-- We can't just call wait_for_hot_stats() at this point, because we only
+-- transmit stats when the session goes idle, and we probably didn't
+-- transmit the last couple of counts yet thanks to the rate-limiting logic
+-- in pgstat_report_stat(). But instead of waiting for the rate limiter's
+-- timeout to elapse, let's just start a new session. The old one will
+-- then send its stats before dying.
+\c -
+
+SELECT wait_for_hot_stats();
+SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
+
+DROP TABLE brin_hot;
+DROP FUNCTION wait_for_hot_stats();
--
2.31.1
OK,
I've polished the last version of the patch a bit (added a regression
test with update of attribute in index predicate and docs about the new
flag into indexam.sgml) and pushed.
I wonder if we could/should improve handling of index predicates. In
particular, it seems to me we could simply ignore indexes when the new
row does not match the index predicate. For example, if there's an index
CREATE INDEX ON t (a) WHERE b = 1;
and the update does:
UPDATE t SET b = 2 WHERE ...;
then we'll not add the tuple pointer to this index anyway, and we could
simply ignore this index when considering HOT. But I might be missing
something important about HOT ...
The main problem I see with this is it requires evaluating the index
predicate for each tuple, which makes it incompatible with the caching
in RelationGetIndexAttrBitmap. Just ditching the caching seems like a
bad idea, so we'd probably have to do this in two phases:
1) Do what we do now, i.e. RelationGetIndexAttrBitmap considering all
indexes / attributes. If this says HOT is possible, great - we're done.
2) If (1) says HOT is not possible, we need to look whether it's because
of regular or partial index. For regular indexes it's clear, for partial
indexes we could ignore this if the predicate evaluates to false for the
new row.
But even if such optimization is possible, it's way out of scope of this
patch and it's not clear to me it's actually a sensible trade-off.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
út 30. 11. 2021 v 20:11 odesílatel Tomas Vondra
<tomas.vondra@enterprisedb.com> napsal:
OK,
I've polished the last version of the patch a bit (added a regression
test with update of attribute in index predicate and docs about the new
flag into indexam.sgml) and pushed.
Thanks a lot for taking over this, improving and pushing!
Show quoted text
I wonder if we could/should improve handling of index predicates. In
particular, it seems to me we could simply ignore indexes when the new
row does not match the index predicate. For example, if there's an indexCREATE INDEX ON t (a) WHERE b = 1;
and the update does:
UPDATE t SET b = 2 WHERE ...;
then we'll not add the tuple pointer to this index anyway, and we could
simply ignore this index when considering HOT. But I might be missing
something important about HOT ...The main problem I see with this is it requires evaluating the index
predicate for each tuple, which makes it incompatible with the caching
in RelationGetIndexAttrBitmap. Just ditching the caching seems like a
bad idea, so we'd probably have to do this in two phases:1) Do what we do now, i.e. RelationGetIndexAttrBitmap considering all
indexes / attributes. If this says HOT is possible, great - we're done.2) If (1) says HOT is not possible, we need to look whether it's because
of regular or partial index. For regular indexes it's clear, for partial
indexes we could ignore this if the predicate evaluates to false for the
new row.But even if such optimization is possible, it's way out of scope of this
patch and it's not clear to me it's actually a sensible trade-off.regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 30, 2021 at 08:11:03PM +0100, Tomas Vondra wrote:
OK,
I've polished the last version of the patch a bit (added a regression test
with update of attribute in index predicate and docs about the new flag into
indexam.sgml) and pushed.
brin.sql's new brin_hot test is failing sometimes.
I saw a local failure and then found this.
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=serinus&dt=2021-12-01%2003%3A00%3A07
SELECT pg_stat_get_tuples_hot_updated('brin_hot'::regclass::oid);
pg_stat_get_tuples_hot_updated
--------------------------------
- 1
+ 0
(1 row)Evidently because:
| 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG: wait_for_hot_stats delayed 33.217301 seconds
It seems like maybe the UDP packet lost to the stats collector got lost ?
It fails less than 10% of the time here, probably depending on load.
BTW there's a typo in brin.sql: precicates
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
brin.sql's new brin_hot test is failing sometimes.
Evidently because:
| 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG: wait_for_hot_stats delayed 33.217301 seconds
It seems like maybe the UDP packet lost to the stats collector got lost ?
It fails less than 10% of the time here, probably depending on load.
Oh, geez. *Please* let us not add another regression failure mode
like the ones that afflict stats.sql. We do not need a doubling
of that failure rate. I suggest just removing this test.
regards, tom lane
On 12/5/21 21:16, Tom Lane wrote:
Justin Pryzby <pryzby@telsasoft.com> writes:
brin.sql's new brin_hot test is failing sometimes.
Evidently because:
| 2021-12-01 04:02:01.096 CET [61a6e587.3106b1:4] LOG: wait_for_hot_stats delayed 33.217301 seconds
It seems like maybe the UDP packet lost to the stats collector got lost ?
It fails less than 10% of the time here, probably depending on load.Oh, geez. *Please* let us not add another regression failure mode
like the ones that afflict stats.sql. We do not need a doubling
of that failure rate. I suggest just removing this test.
Whooops. Agreed, I'll get rid of that test.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
On 12/5/21 21:16, Tom Lane wrote:
Oh, geez. *Please* let us not add another regression failure mode
like the ones that afflict stats.sql. We do not need a doubling
of that failure rate. I suggest just removing this test.
Whooops. Agreed, I'll get rid of that test.
Another idea, perhaps, is to shove that test into stats.sql,
where people would know to ignore it? (Actually, I've thought
more than once that we should mark stats.sql as ignorable
in the schedule ...)
regards, tom lane
On 12/6/21 02:47, Tom Lane wrote:
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
On 12/5/21 21:16, Tom Lane wrote:
Oh, geez. *Please* let us not add another regression failure mode
like the ones that afflict stats.sql. We do not need a doubling
of that failure rate. I suggest just removing this test.Whooops. Agreed, I'll get rid of that test.
Another idea, perhaps, is to shove that test into stats.sql,
where people would know to ignore it? (Actually, I've thought
more than once that we should mark stats.sql as ignorable
in the schedule ...)
Yep. I've moved the test to stats.sql - that seems better than just
ditching it, because we're experimenting with maybe relaxing the HOT
rules for BRIN a bit further and not having tests for that would be
unfortunate.
I haven't marked the test as ignorable. I wonder if we should make that
customizable, so that some animals (like serinus, which fails because of
stats.sql from time to time) could run ignore it. But if it fails
elsewhere it would still be considered a proper failure.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company