diff --git a/contrib/amcheck/meson.build b/contrib/amcheck/meson.build index d5137ef691d..d31dfe99be6 100644 --- a/contrib/amcheck/meson.build +++ b/contrib/amcheck/meson.build @@ -50,6 +50,7 @@ tests += { 't/004_verify_nbtree_unique.pl', 't/005_pitr.pl', 't/006_verify_gin.pl', + 't/007_catalog_reindex_hot.pl', ], }, } diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index dd2218bbbaf..0f62e4989e8 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -1155,6 +1155,7 @@ heapam_index_build_range_scan(Relation heapRelation, BlockNumber previous_blkno = InvalidBlockNumber; BlockNumber root_blkno = InvalidBlockNumber; OffsetNumber root_offsets[MaxHeapTuplesPerPage]; + bool indexed_root[MaxHeapTuplesPerPage]; /* * sanity checks @@ -1331,9 +1332,15 @@ heapam_index_build_range_scan(Relation heapRelation, * the chain root locations won't, so this info doesn't need to be * rebuilt after waiting for another transaction. * - * Note the implied assumption that there is no more than one live - * tuple per HOT-chain --- else we could create more than one index - * entry pointing to the same root tuple. + * That liveness change is not hypothetical. System catalog + * modifications release their relation lock before commit, so a + * non-unique index build can observe both a still-live root and a + * heap-only HOT update from another transaction. Indexing both + * would emit two entries for the same root TID. We do not wait for + * the inserting transaction: that reintroduces VACUUM FULL/CLUSTER + * deadlocks on catalogs (commit 1ddc2703a936). Instead, + * indexed_root[] records which root offsets we already handed to the + * AM on this page. */ if (hscan->rs_cblock != root_blkno) { @@ -1343,6 +1350,7 @@ heapam_index_build_range_scan(Relation heapRelation, heap_get_root_tuples(page, root_offsets); LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_UNLOCK); + MemSet(indexed_root, 0, sizeof(indexed_root)); root_blkno = hscan->rs_cblock; } @@ -1445,6 +1453,12 @@ heapam_index_build_range_scan(Relation heapRelation, * such a tuple could lead to a bogus uniqueness * failure. In that case we wait for the inserting * transaction to finish and check again. + * + * We do not wait merely because the tuple is part of + * a HOT chain. That would re-introduce catalog + * deadlocks with VACUUM FULL/CLUSTER (1ddc2703a936). + * Duplicate root TIDs from a mid-scan HOT update are + * suppressed via indexed_root[] instead. */ if (checking_uniqueness) { @@ -1616,52 +1630,77 @@ heapam_index_build_range_scan(Relation heapRelation, * pass the values[] and isnull[] arrays, instead. */ - if (HeapTupleIsHeapOnly(heapTuple)) { - /* - * For a heap-only tuple, pretend its TID is that of the root. See - * src/backend/access/heap/README.HOT for discussion. - */ ItemPointerData tid; - OffsetNumber offnum; - - offnum = ItemPointerGetOffsetNumber(&heapTuple->t_self); + ItemPointer tidptr; + OffsetNumber emitoff; - /* - * If a HOT tuple points to a root that we don't know about, - * obtain root items afresh. If that still fails, report it as - * corruption. - */ - if (root_offsets[offnum - 1] == InvalidOffsetNumber) + if (HeapTupleIsHeapOnly(heapTuple)) { - Page page = BufferGetPage(hscan->rs_cbuf); + /* + * For a heap-only tuple, pretend its TID is that of the root. + * See src/backend/access/heap/README.HOT for discussion. + */ + OffsetNumber offnum; - LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE); - heap_get_root_tuples(page, root_offsets); - LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_UNLOCK); - } + offnum = ItemPointerGetOffsetNumber(&heapTuple->t_self); - if (!OffsetNumberIsValid(root_offsets[offnum - 1])) - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg_internal("failed to find parent tuple for heap-only tuple at (%u,%u) in table \"%s\"", - ItemPointerGetBlockNumber(&heapTuple->t_self), - offnum, - RelationGetRelationName(heapRelation)))); + /* + * If a HOT tuple points to a root that we don't know about, + * obtain root items afresh. If that still fails, report it + * as corruption. + */ + if (root_offsets[offnum - 1] == InvalidOffsetNumber) + { + Page page = BufferGetPage(hscan->rs_cbuf); - ItemPointerSet(&tid, ItemPointerGetBlockNumber(&heapTuple->t_self), - root_offsets[offnum - 1]); + LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE); + heap_get_root_tuples(page, root_offsets); + LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_UNLOCK); - /* Call the AM's callback routine to process the tuple */ - callback(indexRelation, &tid, values, isnull, tupleIsAlive, + /* + * Leave indexed_root[] unchanged; it tracks TIDs we + * already emitted on this page. + */ + } + + if (!OffsetNumberIsValid(root_offsets[offnum - 1])) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("failed to find parent tuple for heap-only tuple at (%u,%u) in table \"%s\"", + ItemPointerGetBlockNumber(&heapTuple->t_self), + offnum, + RelationGetRelationName(heapRelation)))); + + ItemPointerSet(&tid, ItemPointerGetBlockNumber(&heapTuple->t_self), + root_offsets[offnum - 1]); + tidptr = &tid; + emitoff = root_offsets[offnum - 1]; + } + else + { + tidptr = &heapTuple->t_self; + emitoff = ItemPointerGetOffsetNumber(&heapTuple->t_self); + } + + /* + * HOT chains are confined to one page and must produce at most + * one index entry. If we already emitted this root TID on the + * current page, skip; the earlier callback used the same TID + * (and, for a well-formed HOT chain, the same key). + * + * Record the emission only here, after the partial-index + * predicate has accepted the tuple, so a rejected member does not + * suppress a later one. + */ + Assert(OffsetNumberIsValid(emitoff)); + if (indexed_root[emitoff - 1]) + continue; + indexed_root[emitoff - 1] = true; + + callback(indexRelation, tidptr, values, isnull, tupleIsAlive, callback_state); } - else - { - /* Call the AM's callback routine to process the tuple */ - callback(indexRelation, &heapTuple->t_self, values, isnull, - tupleIsAlive, callback_state); - } } /* Report scan progress one last time. */ diff --git a/contrib/amcheck/t/007_catalog_reindex_hot.pl b/contrib/amcheck/t/007_catalog_reindex_hot.pl new file mode 100644 index 00000000000..69542b8337c --- /dev/null +++ b/contrib/amcheck/t/007_catalog_reindex_hot.pl @@ -0,0 +1,128 @@ + +# Copyright (c) 2026, PostgreSQL Global Development Group + +# BUG #19620: a REINDEX of a non-unique catalog index can emit two btree +# entries for the same heap TID when it races with a HOT update. Catalog +# writers release their lock before commit, so the index build (which holds +# ShareLock) can still see INSERT_IN_PROGRESS heap-only tuples. Waiting for +# those inserts would deadlock VACUUM FULL/CLUSTER on catalogs (1ddc2703); +# heapam_index_build_range_scan instead skips a second emission of the same +# root TID. +# +# pg_class_tblspc_relfilenode_index is the usual casualty: it is not unique, +# so the build does not wait on INSERT_IN_PROGRESS. +# +# The race is easy to miss on an empty initdb (tiny pg_class, scan finishes +# in microseconds). Inflate pg_class, then hammer GRANT/REVOKE on one +# persistent row (relacl is not indexed, so the update is HOT) against +# REINDEX. Unpatched cassert traps in comparetup_index_btree_tiebreak +# ("ItemPointer values should never be equal"). + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; + +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('catalog_reindex_hot'); +$node->init; +$node->append_conf('postgresql.conf', + 'lock_timeout = ' . (1000 * $PostgreSQL::Test::Utils::timeout_default)); +$node->append_conf('postgresql.conf', 'deadlock_timeout = 1s'); +$node->append_conf('postgresql.conf', 'max_locks_per_transaction = 128'); +$node->start; +$node->safe_psql('postgres', q(CREATE EXTENSION amcheck)); + +# Extra pg_class rows lengthen the index-build heap scan. CREATE TYPE AS () +# is a cheap pg_class insert; COMMIT every batch so we do not exhaust the +# lock table. persist_1 is the HOT target (relacl). +$node->safe_psql( + 'postgres', + q( +CREATE TABLE persist_1(i int); +CREATE PROCEDURE catalog_hot_bloat(n_types int) +LANGUAGE plpgsql AS $$ +DECLARE + i int; +BEGIN + FOR i IN 1..n_types LOOP + EXECUTE format('CREATE TYPE catalog_hot_ty_%s AS ()', i); + IF i % 40 = 0 THEN + COMMIT; + END IF; + END LOOP; + COMMIT; +END; +$$; +CALL catalog_hot_bloat(8000); +)); + +# +# VACUUM FULL must not wait out an in-progress catalog insert. CREATE TABLE +# inserts into pg_class and then releases that lock before commit; the rewrite +# should finish while the inserting transaction is still open. +# +# While it runs, the inserting backend must still be able to do more catalog +# work. Waiting for INSERT_IN_PROGRESS during the rewrite deadlocks here: +# VACUUM FULL holds AccessExclusiveLock and waits for the inserter; the +# inserter waits for that exclusive lock. +# +my $hold = $node->background_psql('postgres'); +$hold->query_safe(q(BEGIN; CREATE TABLE hold_open(i int);)); + +my $vf = $node->background_psql('postgres'); +$vf->query_until( + qr/start/, + q( +\echo start +SET statement_timeout = '30s'; +VACUUM FULL pg_class; +)); +$hold->query_safe(q(CREATE TABLE hold_open_2(i int);)); +$vf->query_safe(q(SELECT 1)); +pass('VACUUM FULL pg_class does not deadlock with in-progress catalog inserts'); + +$hold->query_safe(q(COMMIT; DROP TABLE hold_open, hold_open_2;)); +$hold->quit; +$vf->quit; + +# +# Stress REINDEX against concurrent catalog HOT updates of a single pg_class +# row. Serialize GRANT vs GRANT with an advisory lock so pgbench is not +# aborted by "tuple concurrently updated"; REINDEX (ShareLock) still races +# with GRANT (RowExclusiveLock). +# +$node->pgbench( + '--no-vacuum --client=4 --jobs=4 --time=12', + 0, + [qr{actually processed}], + [qr{^$}], + 'concurrent catalog REINDEX and HOT GRANT', + { + '007_reindex_catalog' => q( + REINDEX INDEX pg_class_tblspc_relfilenode_index; + ), + '007_catalog_acl_hot' => q( + SELECT pg_try_advisory_lock(43)::integer AS gotlock \gset + \if :gotlock + GRANT SELECT ON persist_1 TO PUBLIC; + REVOKE SELECT ON persist_1 FROM PUBLIC; + SELECT pg_advisory_unlock(43); + \endif + ) + }); + +$node->safe_psql( + 'postgres', + q( +SELECT bt_index_check('pg_class_tblspc_relfilenode_index', true); +SELECT bt_index_check('pg_class_relname_nsp_index', true); +SELECT bt_index_check('pg_class_oid_index', true); +SELECT bt_index_parent_check('pg_class_tblspc_relfilenode_index', true, true); +)); +pass('pg_class indexes pass bt_index_check after concurrent REINDEX'); + +$node->stop; +done_testing();