On Mon, Sep 23, 2024 at 04:00:00PM +0300, Alexander Lakhin wrote:
> I tested it with two code modifications (1st is to make each created
> expression index TOASTed (by prepending 1M of spaces to the indexeprs
> value) and 2nd to make each created index an expression index (by
> modifying index_elem_options in gram.y) - both modifications are kludgy so
> I don't dare to publish them) and found no other snapshot-related issues
> during `make check-world`.
Thanks. Here is an updated patch with tests and comments. I've also moved
the calls to PushActiveSnapshot()/PopActiveSnapshot() to surround only the
section of code where the snapshot is needed. Besides being more similar
in style to other fixes I found, I think this is safer because much of this
code is cautious to avoid deadlocks. For example, DefineIndex() has the
following comment:
/*
* The snapshot subsystem could still contain registered snapshots that
* are holding back our process's advertised xmin; in particular, if
* default_transaction_isolation = serializable, there is a transaction
* snapshot that is still active. The CatalogSnapshot is likewise a
* hazard. To ensure no deadlocks, we must commit and start yet another
* transaction, and do our wait before any snapshot has been taken in
it.
*/
I carefully inspected all the code paths this patch touches, and I think
I've got all the details right, but I would be grateful if someone else
could take a look.
--
nathan
>From 3be62fac910e930f5635193ac60d1536b17e0d6d Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Mon, 23 Sep 2024 10:33:58 -0500
Subject: [PATCH v3 1/1] Ensure we have a snapshot when updating pg_index
entries.
Creating, reindexing, and dropping an index concurrently could
entail accessing pg_index's TOAST table, which was recently added
in commit b52c4fc3c0. These code paths start and commit their own
transactions internally, but they do not always set an active
snapshot. This rightfully leads to assertion failures and ERRORs
when trying to access pg_index's TOAST table, such as the
following:
ERROR: cannot fetch toast data without an active snapshot
To fix, push an active snapshot just before each section of code
that might require accessing pg_index's TOAST table, and pop it
shortly afterwards.
Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/a97d7401-e7c9-f771-6a00-037379f0a8bb%40gmail.com
---
src/backend/catalog/index.c | 21 +++++++++++++++++++++
src/backend/commands/indexcmds.c | 24 ++++++++++++++++++++++++
src/test/regress/expected/indexing.out | 15 +++++++++++++++
src/test/regress/sql/indexing.sql | 16 ++++++++++++++++
4 files changed, 76 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b2b3ecb524..09d79b26b8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2276,9 +2276,17 @@ index_drop(Oid indexId, bool concurrent, bool
concurrent_lock_mode)
*/
WaitForLockers(heaplocktag, AccessExclusiveLock, true);
+ /*
+ * Updating pg_index might involve TOAST table access, so
ensure we
+ * have a valid snapshot.
+ */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
/* Finish invalidation of index and mark it as dead */
index_concurrently_set_dead(heapId, indexId);
+ PopActiveSnapshot();
+
/*
* Again, commit the transaction to make the pg_index update
visible
* to other sessions.
@@ -2326,6 +2334,16 @@ index_drop(Oid indexId, bool concurrent, bool
concurrent_lock_mode)
RelationForgetRelation(indexId);
+ /*
+ * Updating pg_index might involve TOAST table access, so ensure we
have a
+ * valid snapshot. We only expect to get here without an active
snapshot
+ * in the concurrent path.
+ */
+ if (concurrent)
+ PushActiveSnapshot(GetTransactionSnapshot());
+ else
+ Assert(ActiveSnapshotSet());
+
/*
* fix INDEX relation, and check for expressional index
*/
@@ -2343,6 +2361,9 @@ index_drop(Oid indexId, bool concurrent, bool
concurrent_lock_mode)
ReleaseSysCache(tuple);
table_close(indexRelation, RowExclusiveLock);
+ if (concurrent)
+ PopActiveSnapshot();
+
/*
* if it has any expression columns, we might have stored statistics
about
* them.
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index f99c2d2dee..7d41cb73ab 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1798,11 +1798,19 @@ DefineIndex(Oid tableId,
PROGRESS_CREATEIDX_PHASE_WAIT_3);
WaitForOlderSnapshots(limitXmin, true);
+ /*
+ * Updating pg_index might involve TOAST table access, so ensure we
have a
+ * valid snapshot.
+ */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
/*
* Index can now be marked valid -- update its pg_index entry
*/
index_set_state_flags(indexRelationId, INDEX_CREATE_SET_VALID);
+ PopActiveSnapshot();
+
/*
* The pg_index update will cause backends (including this one) to
update
* relcache entries for the index itself, but we should also send a
@@ -4256,12 +4264,20 @@ ReindexRelationConcurrently(const ReindexStmt *stmt,
Oid relationOid, const Rein
get_rel_namespace(oldidx->tableId),
false);
+ /*
+ * Updating pg_index might involve TOAST table access, so
ensure we
+ * have a valid snapshot.
+ */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
/*
* Swap old index with the new one. This also marks the new
one as
* valid and the old one as not valid.
*/
index_concurrently_swap(newidx->indexId, oldidx->indexId,
oldName);
+ PopActiveSnapshot();
+
/*
* Invalidate the relcache for the table, so that after this
commit
* all sessions will refresh any cached plans that might
reference the
@@ -4312,7 +4328,15 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid
relationOid, const Rein
*/
CHECK_FOR_INTERRUPTS();
+ /*
+ * Updating pg_index might involve TOAST table access, so
ensure we
+ * have a valid snapshot.
+ */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
index_concurrently_set_dead(oldidx->tableId, oldidx->indexId);
+
+ PopActiveSnapshot();
}
/* Commit this transaction to make the updates visible. */
diff --git a/src/test/regress/expected/indexing.out
b/src/test/regress/expected/indexing.out
index f25723da92..49f92fe81f 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1640,3 +1640,18 @@ select indexrelid::regclass, indisvalid, indisreplident,
(3 rows)
drop table parted_replica_tab;
+-- indexing commands work with TOASTed values in pg_index
+create table test_pg_index_toast_table (a int);
+create or replace function test_pg_index_toast_func (a int, b int[])
+ returns bool as $$ select true $$ language sql immutable;
+select array_agg(n) b from generate_series(1, 10000) n \gset
+create index concurrently test_pg_index_toast_index
+ on test_pg_index_toast_table (test_pg_index_toast_func(a, :'b'));
+reindex index concurrently test_pg_index_toast_index;
+drop index concurrently test_pg_index_toast_index;
+create index test_pg_index_toast_index
+ on test_pg_index_toast_table (test_pg_index_toast_func(a, :'b'));
+reindex index test_pg_index_toast_index;
+drop index test_pg_index_toast_index;
+drop function test_pg_index_toast_func;
+drop table test_pg_index_toast_table;
diff --git a/src/test/regress/sql/indexing.sql
b/src/test/regress/sql/indexing.sql
index 5f1f4b80c9..8c07d49d46 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -919,3 +919,19 @@ select indexrelid::regclass, indisvalid, indisreplident,
where indexrelid::regclass::text like 'parted_replica%'
order by indexrelid::regclass::text collate "C";
drop table parted_replica_tab;
+
+-- indexing commands work with TOASTed values in pg_index
+create table test_pg_index_toast_table (a int);
+create or replace function test_pg_index_toast_func (a int, b int[])
+ returns bool as $$ select true $$ language sql immutable;
+select array_agg(n) b from generate_series(1, 10000) n \gset
+create index concurrently test_pg_index_toast_index
+ on test_pg_index_toast_table (test_pg_index_toast_func(a, :'b'));
+reindex index concurrently test_pg_index_toast_index;
+drop index concurrently test_pg_index_toast_index;
+create index test_pg_index_toast_index
+ on test_pg_index_toast_table (test_pg_index_toast_func(a, :'b'));
+reindex index test_pg_index_toast_index;
+drop index test_pg_index_toast_index;
+drop function test_pg_index_toast_func;
+drop table test_pg_index_toast_table;
--
2.39.5 (Apple Git-154)
