From cfa6e309b053319684f9b7ab22173e603dc69916 Mon Sep 17 00:00:00 2001 From: Manuel Reyes Bravo Date: Fri, 2 Oct 2026 11:16:32 -0300 Subject: [PATCH v4] Fix index corruption after rolling back ALTER TABLE SET TABLESPACE ALTER TABLE ... SET TABLESPACE rewrites a table's heap to a new relfilenode but leaves the table's indexes on their existing relfilenodes. The two then roll back by different mechanisms: on abort the heap's new file is discarded, freeing the heap TIDs consumed by rows inserted after the SET TABLESPACE, while the index entries for those rows were written to the unchanged index files and survive. A later insert can reuse a freed heap TID, leaving two index entries that point at the same live heap tuple. This surfaces as a _bt_posting_valid assertion failure in nbtree deduplication, and as duplicate rows through an index-only scan in gist. A TRUNCATE after the move in the same transaction has a similar problem: it truncates the old index files in place, which a rollback cannot undo. The indexes only need to share the heap's rewrite when the table is modified again in the same transaction, which is also the only time the corruption can arise. So leave SET TABLESPACE itself alone, and give an index a new relfilenumber -- copying it within its own tablespace, as documented, the indexes stay where they are -- when the executor opens it to modify a table whose storage was replaced in the current transaction, and before an in-place TRUNCATE of such a table. The ALTER alone in its transaction, the common case, copies nothing and needs no extra space where the indexes live. The regression test checks that the move alone leaves the index file untouched, and that after inserting enough rows inside the transaction to grow the index, with and without a subtransaction, or truncating the table, a rollback leaves the index file at its pre-transaction size. Bug: #19686 Reported-by: Alexander Lakhin Reviewed-by: Alexandre Felipe Reviewed-by: Shihao Zhong --- src/backend/commands/tablecmds.c | 93 ++++++++++++++++++++++++ src/backend/executor/execIndexing.c | 9 +++ src/include/commands/tablecmds.h | 1 + src/test/regress/expected/tablespace.out | 65 +++++++++++++++++ src/test/regress/sql/tablespace.sql | 36 +++++++++ 5 files changed, 204 insertions(+) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fd144d783d9..4f771cbbe9f 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2241,6 +2241,24 @@ ExecuteTruncateGuts(List *explicit_rels, if (rel->rd_createSubid == mySubid || rel->rd_newRelfilelocatorSubid == mySubid) { + List *indexIds = RelationGetIndexList(rel); + ListCell *ic; + + /* + * The in-place truncation below also truncates the indexes in + * place. An index still on the file it had before this + * transaction (SET TABLESPACE leaves them so) must get a new one + * first, or a rollback could not restore it. + */ + foreach(ic, indexIds) + { + Relation indexRel = index_open(lfirst_oid(ic), AccessExclusiveLock); + + IndexFollowHeapRelfilelocator(rel, indexRel); + index_close(indexRel, NoLock); + } + list_free(indexIds); + /* Immediate, non-rollbackable truncation is OK */ heap_truncate_one_rel(rel); } @@ -17877,6 +17895,81 @@ index_copy_data(Relation rel, RelFileLocator newrlocator) smgrclose(dstrel); } +/* + * IndexFollowHeapRelfilelocator + * + * ALTER TABLE SET TABLESPACE gives a table's heap a new relfilenumber but + * leaves the table's indexes on their old files, since the indexes are + * documented to stay where they are. That mix is fine until the table is + * modified again in the same transaction: on abort the heap's new file is + * discarded, freeing the heap TIDs used by rows inserted after the move, + * while the index entries for those rows were written to the old index files + * and survive, so a later insert can reuse a freed TID and leave two index + * entries pointing at one live tuple. + * + * So, before an index is modified (or truncated in place) in a transaction + * that replaced its heap's storage, give the index a new relfilenumber too, + * copying it within its own tablespace: an abort then discards the new heap + * and index files together. Doing this here rather than in SET TABLESPACE + * itself keeps the common case -- the ALTER alone in its transaction -- + * free of any copy, so it needs no extra space where the indexes live. + * + * rd_firstRelfilelocatorSubid says whether a relation's storage differs from + * what it was at the start of the top transaction; it is kept accurate for + * RelationNeedsWAL(). An index created, or given a new file, in this + * transaction already shares the heap's fate and is left alone. + */ +void +IndexFollowHeapRelfilelocator(Relation heapRel, Relation indexRel) +{ + RelFileNumber newrelfilenumber; + RelFileLocator newrlocator; + Relation pg_class; + HeapTuple tuple; + ItemPointerData otid; + Form_pg_class rd_rel; + + if (heapRel->rd_firstRelfilelocatorSubid == InvalidSubTransactionId || + indexRel->rd_firstRelfilelocatorSubid != InvalidSubTransactionId || + indexRel->rd_createSubid != InvalidSubTransactionId) + return; + + /* Only plain indexes have storage that can hold the stale entries. */ + if (indexRel->rd_rel->relkind != RELKIND_INDEX || + !RELKIND_HAS_STORAGE(indexRel->rd_rel->relkind)) + return; + + /* Allocate a new relfilenumber in the index's current tablespace. */ + newrelfilenumber = GetNewRelFileNumber(indexRel->rd_rel->reltablespace, NULL, + indexRel->rd_rel->relpersistence); + newrlocator = indexRel->rd_locator; + newrlocator.relNumber = newrelfilenumber; + + /* Copy the index into the new file and schedule the old one for cleanup. */ + index_copy_data(indexRel, newrlocator); + + /* Update the pg_class row; only the relfilenode changes. */ + pg_class = table_open(RelationRelationId, RowExclusiveLock); + tuple = SearchSysCacheLockedCopy1(RELOID, + ObjectIdGetDatum(RelationGetRelid(indexRel))); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for index %u", + RelationGetRelid(indexRel)); + otid = tuple->t_self; + rd_rel = (Form_pg_class) GETSTRUCT(tuple); + rd_rel->relfilenode = newrelfilenumber; + CatalogTupleUpdate(pg_class, &otid, tuple); + UnlockTuple(pg_class, &otid, InplaceUpdateTupleLock); + heap_freetuple(tuple); + table_close(pg_class, RowExclusiveLock); + + InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(indexRel), 0); + RelationAssumeNewRelfilelocator(indexRel); + + /* Make the relfilenode change visible. */ + CommandCounterIncrement(); +} + /* * ALTER TABLE ENABLE/DISABLE TRIGGER * diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index eb383812901..b9695146363 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -111,6 +111,7 @@ #include "access/tableam.h" #include "access/xact.h" #include "catalog/index.h" +#include "commands/tablecmds.h" #include "executor/executor.h" #include "nodes/nodeFuncs.h" #include "storage/lmgr.h" @@ -212,6 +213,14 @@ ExecOpenIndices(ResultRelInfo *resultRelInfo, bool speculative) indexDesc = index_open(indexOid, RowExclusiveLock); + /* + * If the table's storage was replaced earlier in this transaction + * while the index kept its old file (ALTER TABLE SET TABLESPACE moves + * the heap only), the index must get a new file too before we write + * to it, so that an abort discards both together. + */ + IndexFollowHeapRelfilelocator(resultRelation, indexDesc); + /* extract index key information from the index's pg_index info */ ii = BuildIndexInfo(indexDesc); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index c3d8518cb62..dfbe4e4a775 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -69,6 +69,7 @@ extern void ExecuteTruncateGuts(List *explicit_rels, extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass); extern bool CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId); +extern void IndexFollowHeapRelfilelocator(Relation heapRel, Relation indexRel); extern void SetRelationTableSpace(Relation rel, Oid newTableSpaceId, RelFileNumber newRelFilenumber); diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out index f0dd25cdf0c..282956a8f62 100644 --- a/src/test/regress/expected/tablespace.out +++ b/src/test/regress/expected/tablespace.out @@ -951,6 +951,71 @@ ERROR: permission denied for tablespace regress_tblspace REINDEX (TABLESPACE regress_tblspace, CONCURRENTLY) TABLE tablespace_table; -- fail ERROR: permission denied for tablespace regress_tblspace RESET ROLE; +-- ALTER TABLE SET TABLESPACE moves the heap only; the indexes keep their files +-- unless the table is modified later in the same transaction, in which case +-- they get new files too, so that a rollback discards all of them together. +CREATE TABLE tbspace_rollback (a int); +INSERT INTO tbspace_rollback SELECT generate_series(1, 100); +CREATE INDEX tbspace_rollback_idx ON tbspace_rollback (a); +SELECT pg_relation_size('tbspace_rollback_idx') AS idx_size_before \gset +SELECT pg_relation_filepath('tbspace_rollback_idx') AS idx_path_before \gset +-- the move alone leaves the index file untouched +ALTER TABLE tbspace_rollback SET TABLESPACE regress_tblspace; +SELECT pg_relation_filepath('tbspace_rollback_idx') = :'idx_path_before' AS idx_untouched; + idx_untouched +--------------- + t +(1 row) + +-- rows inserted after the move: enough to grow the index by at least a page +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +INSERT INTO tbspace_rollback SELECT generate_series(101, 2000); +SELECT pg_relation_size('tbspace_rollback_idx') > :idx_size_before AS idx_grew; + idx_grew +---------- + t +(1 row) + +ROLLBACK; +SELECT pg_relation_size('tbspace_rollback_idx') = :idx_size_before AS idx_size_restored; + idx_size_restored +------------------- + t +(1 row) + +-- the same across a subtransaction +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +SAVEPOINT s; +INSERT INTO tbspace_rollback SELECT generate_series(101, 2000); +ROLLBACK TO s; +INSERT INTO tbspace_rollback SELECT generate_series(101, 2000); +ROLLBACK; +SELECT pg_relation_size('tbspace_rollback_idx') = :idx_size_before AS idx_size_restored; + idx_size_restored +------------------- + t +(1 row) + +-- TRUNCATE after the move is done in place; it must not truncate the old file +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +TRUNCATE tbspace_rollback; +ROLLBACK; +SELECT pg_relation_size('tbspace_rollback_idx') = :idx_size_before AS idx_size_restored; + idx_size_restored +------------------- + t +(1 row) + +SELECT count(*) FROM tbspace_rollback; + count +------- + 100 +(1 row) + +DROP TABLE tbspace_rollback; ALTER TABLESPACE regress_tblspace RENAME TO regress_tblspace_renamed; ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default; ALTER INDEX ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default; diff --git a/src/test/regress/sql/tablespace.sql b/src/test/regress/sql/tablespace.sql index c43a59e5957..c711b0cbacc 100644 --- a/src/test/regress/sql/tablespace.sql +++ b/src/test/regress/sql/tablespace.sql @@ -420,6 +420,42 @@ REINDEX (TABLESPACE regress_tblspace) TABLE tablespace_table; -- fail REINDEX (TABLESPACE regress_tblspace, CONCURRENTLY) TABLE tablespace_table; -- fail RESET ROLE; +-- ALTER TABLE SET TABLESPACE moves the heap only; the indexes keep their files +-- unless the table is modified later in the same transaction, in which case +-- they get new files too, so that a rollback discards all of them together. +CREATE TABLE tbspace_rollback (a int); +INSERT INTO tbspace_rollback SELECT generate_series(1, 100); +CREATE INDEX tbspace_rollback_idx ON tbspace_rollback (a); +SELECT pg_relation_size('tbspace_rollback_idx') AS idx_size_before \gset +SELECT pg_relation_filepath('tbspace_rollback_idx') AS idx_path_before \gset +-- the move alone leaves the index file untouched +ALTER TABLE tbspace_rollback SET TABLESPACE regress_tblspace; +SELECT pg_relation_filepath('tbspace_rollback_idx') = :'idx_path_before' AS idx_untouched; +-- rows inserted after the move: enough to grow the index by at least a page +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +INSERT INTO tbspace_rollback SELECT generate_series(101, 2000); +SELECT pg_relation_size('tbspace_rollback_idx') > :idx_size_before AS idx_grew; +ROLLBACK; +SELECT pg_relation_size('tbspace_rollback_idx') = :idx_size_before AS idx_size_restored; +-- the same across a subtransaction +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +SAVEPOINT s; +INSERT INTO tbspace_rollback SELECT generate_series(101, 2000); +ROLLBACK TO s; +INSERT INTO tbspace_rollback SELECT generate_series(101, 2000); +ROLLBACK; +SELECT pg_relation_size('tbspace_rollback_idx') = :idx_size_before AS idx_size_restored; +-- TRUNCATE after the move is done in place; it must not truncate the old file +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +TRUNCATE tbspace_rollback; +ROLLBACK; +SELECT pg_relation_size('tbspace_rollback_idx') = :idx_size_before AS idx_size_restored; +SELECT count(*) FROM tbspace_rollback; +DROP TABLE tbspace_rollback; + ALTER TABLESPACE regress_tblspace RENAME TO regress_tblspace_renamed; ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default; -- 2.55.0