From f818dfd16cd2121b8b85f919d03ea494dd43f6e0 Mon Sep 17 00:00:00 2001 From: Manuel Reyes Bravo Date: Thu, 1 Oct 2026 21:37:04 -0300 Subject: [PATCH v5] 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 truncates the old index files in place, which a rollback cannot undo either. Fix by giving each index of the table a new relfilenumber in the same ALTER, copying it within its own tablespace (the indexes stay where they are, as documented), so that an abort discards the new heap and index files together. Two cases need no copy: - The ALTER is a top-level statement outside a transaction block, so nothing can modify the table before the transaction ends. This requires that no ddl_command_end event trigger runs after it, and the commit is forced right after the statement, as for PreventInTransactionBlock, so that a later statement of an extended-protocol pipeline cannot share the transaction. - The index's current file was created in the current subtransaction, for example an index created, or moved, earlier in it. An abort discards that file together with the heap's new one. This also lets a transaction move the indexes first and then the table without copying the indexes twice. The regression test checks that the ALTER on its own leaves the index files untouched; that in a transaction block a rollback after inserting or truncating leaves the index file at its pre-transaction size; that an index created or moved earlier in the subtransaction is not copied; and that after a rollback to a savepoint, an index created, reindexed or already copied before the savepoint returns no row for the rolled-back values. Bug: #19686 Reported-by: Alexander Lakhin Suggested-by: Andres Freund Reviewed-by: Alexandre Felipe Reviewed-by: Shihao Zhong --- src/backend/commands/tablecmds.c | 132 ++++++++++++++++++++++- src/backend/tcop/utility.c | 1 + src/include/tcop/utility.h | 1 + src/test/regress/expected/tablespace.out | 120 +++++++++++++++++++++ src/test/regress/sql/tablespace.sql | 76 +++++++++++++ 5 files changed, 325 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fd144d783d9..dcfbb362c73 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -98,6 +98,7 @@ #include "tcop/utility.h" #include "utils/acl.h" #include "utils/builtins.h" +#include "utils/evtcache.h" #include "utils/fmgroids.h" #include "utils/inval.h" #include "utils/lsyscache.h" @@ -700,7 +701,10 @@ static void ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel, bool toLogged); static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel, const char *tablespacename, LOCKMODE lockmode); -static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode); +static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode, + bool copyIndexes); +static bool ATSetTableSpaceCopyIndexes(AlterTableUtilityContext *context); +static void ATExecSetTableSpaceNewIndexRelfilenumber(Oid indexOid, LOCKMODE lockmode); static void ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace); static void ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, @@ -6097,7 +6101,8 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, * just do a block-by-block copy. */ if (tab->newTableSpace) - ATExecSetTableSpace(tab->relid, tab->newTableSpace, lockmode); + ATExecSetTableSpace(tab->relid, tab->newTableSpace, lockmode, + ATSetTableSpaceCopyIndexes(context)); } /* @@ -17526,13 +17531,16 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation, * rewriting to be done, so we just want to copy the data as fast as possible. */ static void -ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) +ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode, + bool copyIndexes) { Relation rel; Oid reltoastrelid; + char relkind; RelFileNumber newrelfilenumber; RelFileLocator newrlocator; List *reltoastidxids = NIL; + List *reltabidxids = NIL; ListCell *lc; /* @@ -17550,6 +17558,7 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) } reltoastrelid = rel->rd_rel->reltoastrelid; + relkind = rel->rd_rel->relkind; /* Fetch the list of indexes on toast relation if necessary */ if (OidIsValid(reltoastrelid)) { @@ -17596,6 +17605,14 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) RelationAssumeNewRelfilelocator(rel); + /* + * If this is a table, collect its index list now, while the relation is + * still open, so we can give each index a fresh relfilenumber below. + */ + if (copyIndexes && + (relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW)) + reltabidxids = RelationGetIndexList(rel); + relation_close(rel, NoLock); /* Make sure the reltablespace change is visible */ @@ -17603,12 +17620,117 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode) /* Move associated toast relation and/or indexes, too */ if (OidIsValid(reltoastrelid)) - ATExecSetTableSpace(reltoastrelid, newTableSpace, lockmode); + ATExecSetTableSpace(reltoastrelid, newTableSpace, lockmode, copyIndexes); foreach(lc, reltoastidxids) - ATExecSetTableSpace(lfirst_oid(lc), newTableSpace, lockmode); + ATExecSetTableSpace(lfirst_oid(lc), newTableSpace, lockmode, copyIndexes); /* Clean up */ list_free(reltoastidxids); + + /* + * The heap now has a new relfilenode. Give each of the table's indexes a + * fresh relfilenode too, so that the indexes share the heap's rewrite fate + * across commit and abort. See ATExecSetTableSpaceNewIndexRelfilenumber. + */ + foreach(lc, reltabidxids) + ATExecSetTableSpaceNewIndexRelfilenumber(lfirst_oid(lc), lockmode); + list_free(reltabidxids); +} + +/* + * Does ALTER TABLE SET TABLESPACE need to give the table's indexes new files? + * + * Only if something can still modify the table in the same transaction after + * the move. Nothing can when the ALTER is a top-level statement outside any + * transaction block and nothing runs after it before the commit: no + * ddl_command_end event trigger, and no further statement of an + * extended-protocol pipeline, which would otherwise share the transaction + * until the next Sync. To rule out the latter, force the commit right after + * the ALTER, as PreventInTransactionBlock does. + */ +static bool +ATSetTableSpaceCopyIndexes(AlterTableUtilityContext *context) +{ + if (context == NULL || IsInTransactionBlock(context->isTopLevel)) + return true; + if (EventCacheLookup(EVT_DDLCommandEnd) != NIL) + return true; + + MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT; + return false; +} + +/* + * Give one of a table's indexes a fresh relfilenumber within its existing + * tablespace, copying the current index file to the new relfilenumber. + * + * ATExecSetTableSpace() calls this for each index of a table whose heap it has + * just rewritten to a new relfilenode. The indexes must share the heap's + * transactional fate: an abort has to discard the index entries written during + * the transaction together with the heap's new file, since the heap TIDs those + * entries point at are freed by the abort and can be reused by later inserts. + * Copying the existing index file keeps the added cost close to that of the + * heap move itself; the index stays in its own tablespace, as documented. + */ +static void +ATExecSetTableSpaceNewIndexRelfilenumber(Oid indexOid, LOCKMODE lockmode) +{ + Relation ind; + RelFileNumber newrelfilenumber; + RelFileLocator newrlocator; + Relation pg_class; + HeapTuple tuple; + ItemPointerData otid; + Form_pg_class rd_rel; + + ind = relation_open(indexOid, lockmode); + + /* + * Only plain indexes have storage that can hold the stale entries. An + * index whose current file was created in this subtransaction (the index + * itself, or a new file for it) is discarded together with the heap's new + * file on abort, so it needs no copy. rd_newRelfilelocatorSubid can be zero after + * a rollback to a savepoint even though the file is new; then this falls + * back to rd_createSubid, and copies if that does not match either. + */ + if (ind->rd_rel->relkind != RELKIND_INDEX || + !RELKIND_HAS_STORAGE(ind->rd_rel->relkind) || + Max(ind->rd_newRelfilelocatorSubid, ind->rd_createSubid) == + GetCurrentSubTransactionId()) + { + relation_close(ind, NoLock); + return; + } + + /* Allocate a new relfilenumber in the index's current tablespace. */ + newrelfilenumber = GetNewRelFileNumber(ind->rd_rel->reltablespace, NULL, + ind->rd_rel->relpersistence); + newrlocator = ind->rd_locator; + newrlocator.relNumber = newrelfilenumber; + + /* Copy the index into the new file and schedule the old one for cleanup. */ + index_copy_data(ind, newrlocator); + + /* Update the pg_class row; only the relfilenode changes. */ + pg_class = table_open(RelationRelationId, RowExclusiveLock); + tuple = SearchSysCacheLockedCopy1(RELOID, ObjectIdGetDatum(indexOid)); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for index %u", indexOid); + 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, indexOid, 0); + RelationAssumeNewRelfilelocator(ind); + + relation_close(ind, NoLock); + + /* Make the relfilenode change visible. */ + CommandCounterIncrement(); } /* diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 4d33fcb5e9d..57d88f371ef 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1315,6 +1315,7 @@ ProcessUtilitySlow(ParseState *pstate, atcontext.relid = relid; atcontext.params = params; atcontext.queryEnv = queryEnv; + atcontext.isTopLevel = isTopLevel; /* ... ensure we have an event trigger context ... */ EventTriggerAlterTableStart(parsetree); diff --git a/src/include/tcop/utility.h b/src/include/tcop/utility.h index abbdb401bf6..0357550da71 100644 --- a/src/include/tcop/utility.h +++ b/src/include/tcop/utility.h @@ -34,6 +34,7 @@ typedef struct AlterTableUtilityContext Oid relid; /* OID of ALTER's target table */ ParamListInfo params; /* any parameters available to ALTER TABLE */ QueryEnvironment *queryEnv; /* execution environment for ALTER TABLE */ + bool isTopLevel; /* ALTER TABLE is a top-level statement */ } AlterTableUtilityContext; /* diff --git a/src/test/regress/expected/tablespace.out b/src/test/regress/expected/tablespace.out index f0dd25cdf0c..ccc43061aad 100644 --- a/src/test/regress/expected/tablespace.out +++ b/src/test/regress/expected/tablespace.out @@ -951,6 +951,126 @@ 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 and leaves the indexes in their +-- tablespace. Run on its own, as here, it leaves their files alone too. +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 \gset +ALTER TABLE tbspace_rollback SET TABLESPACE regress_tblspace; +SELECT pg_relation_filepath('tbspace_rollback_idx') = :'idx_path' AS idx_file_kept; + idx_file_kept +--------------- + t +(1 row) + +-- In a transaction block the indexes get new files, so that a rollback +-- discards the entries added after the move together with the heap's file. +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +SELECT pg_relation_filepath('tbspace_rollback_idx') <> :'idx_path' AS idx_file_new; + idx_file_new +-------------- + t +(1 row) + +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 for a TRUNCATE after the move, which is done in place +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) + +-- An index created in the same subtransaction is discarded with the heap +-- anyway and is not copied. Moving the indexes first and then the table +-- copies each index only once. +BEGIN; +CREATE INDEX tbspace_rollback_idx2 ON tbspace_rollback (a); +SELECT pg_relation_filepath('tbspace_rollback_idx2') AS idx2_path \gset +ALTER INDEX tbspace_rollback_idx SET TABLESPACE regress_tblspace; +SELECT pg_relation_filepath('tbspace_rollback_idx') AS idx_moved_path \gset +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +SELECT pg_relation_filepath('tbspace_rollback_idx2') = :'idx2_path' AS idx2_not_copied, + pg_relation_filepath('tbspace_rollback_idx') = :'idx_moved_path' AS idx_not_copied_again; + idx2_not_copied | idx_not_copied_again +-----------------+---------------------- + t | t +(1 row) + +ROLLBACK; +DROP TABLE tbspace_rollback; +-- An index file that is new in this transaction but older than a savepoint +-- survives a rollback to it, so it must still follow a move made after the +-- savepoint: an index created or reindexed before it, or already copied by +-- an earlier move. Rows inserted afterwards reuse the TIDs freed by the +-- rollbacks; a stale index entry would match them. +CREATE TABLE tbspace_subxact (a int); +INSERT INTO tbspace_subxact SELECT generate_series(1, 100); +SET enable_seqscan = off; +SET enable_bitmapscan = off; +BEGIN; +CREATE INDEX tbspace_subxact_idx ON tbspace_subxact (a); +SAVEPOINT s; +ALTER TABLE tbspace_subxact SET TABLESPACE regress_tblspace; +INSERT INTO tbspace_subxact SELECT -generate_series(1, 50); +ROLLBACK TO s; +COMMIT; +BEGIN; +REINDEX INDEX tbspace_subxact_idx; +SAVEPOINT s; +ALTER TABLE tbspace_subxact SET TABLESPACE regress_tblspace; +INSERT INTO tbspace_subxact SELECT -generate_series(1, 50); +ROLLBACK TO s; +COMMIT; +BEGIN; +ALTER TABLE tbspace_subxact SET TABLESPACE regress_tblspace; +INSERT INTO tbspace_subxact VALUES (0); +SAVEPOINT s; +ALTER TABLE tbspace_subxact SET TABLESPACE pg_default; +INSERT INTO tbspace_subxact SELECT -generate_series(1, 50); +ROLLBACK TO s; +COMMIT; +INSERT INTO tbspace_subxact SELECT generate_series(101, 300); +SELECT count(*) FROM tbspace_subxact WHERE a < 0; + count +------- + 0 +(1 row) + +SELECT count(*) FROM tbspace_subxact; + count +------- + 301 +(1 row) + +RESET enable_seqscan; +RESET enable_bitmapscan; +DROP TABLE tbspace_subxact; 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..8fc94894491 100644 --- a/src/test/regress/sql/tablespace.sql +++ b/src/test/regress/sql/tablespace.sql @@ -420,6 +420,82 @@ 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 and leaves the indexes in their +-- tablespace. Run on its own, as here, it leaves their files alone too. +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 \gset +ALTER TABLE tbspace_rollback SET TABLESPACE regress_tblspace; +SELECT pg_relation_filepath('tbspace_rollback_idx') = :'idx_path' AS idx_file_kept; +-- In a transaction block the indexes get new files, so that a rollback +-- discards the entries added after the move together with the heap's file. +BEGIN; +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +SELECT pg_relation_filepath('tbspace_rollback_idx') <> :'idx_path' AS idx_file_new; +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 for a TRUNCATE after the move, which is done in place +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; +-- An index created in the same subtransaction is discarded with the heap +-- anyway and is not copied. Moving the indexes first and then the table +-- copies each index only once. +BEGIN; +CREATE INDEX tbspace_rollback_idx2 ON tbspace_rollback (a); +SELECT pg_relation_filepath('tbspace_rollback_idx2') AS idx2_path \gset +ALTER INDEX tbspace_rollback_idx SET TABLESPACE regress_tblspace; +SELECT pg_relation_filepath('tbspace_rollback_idx') AS idx_moved_path \gset +ALTER TABLE tbspace_rollback SET TABLESPACE pg_default; +SELECT pg_relation_filepath('tbspace_rollback_idx2') = :'idx2_path' AS idx2_not_copied, + pg_relation_filepath('tbspace_rollback_idx') = :'idx_moved_path' AS idx_not_copied_again; +ROLLBACK; +DROP TABLE tbspace_rollback; +-- An index file that is new in this transaction but older than a savepoint +-- survives a rollback to it, so it must still follow a move made after the +-- savepoint: an index created or reindexed before it, or already copied by +-- an earlier move. Rows inserted afterwards reuse the TIDs freed by the +-- rollbacks; a stale index entry would match them. +CREATE TABLE tbspace_subxact (a int); +INSERT INTO tbspace_subxact SELECT generate_series(1, 100); +SET enable_seqscan = off; +SET enable_bitmapscan = off; +BEGIN; +CREATE INDEX tbspace_subxact_idx ON tbspace_subxact (a); +SAVEPOINT s; +ALTER TABLE tbspace_subxact SET TABLESPACE regress_tblspace; +INSERT INTO tbspace_subxact SELECT -generate_series(1, 50); +ROLLBACK TO s; +COMMIT; +BEGIN; +REINDEX INDEX tbspace_subxact_idx; +SAVEPOINT s; +ALTER TABLE tbspace_subxact SET TABLESPACE regress_tblspace; +INSERT INTO tbspace_subxact SELECT -generate_series(1, 50); +ROLLBACK TO s; +COMMIT; +BEGIN; +ALTER TABLE tbspace_subxact SET TABLESPACE regress_tblspace; +INSERT INTO tbspace_subxact VALUES (0); +SAVEPOINT s; +ALTER TABLE tbspace_subxact SET TABLESPACE pg_default; +INSERT INTO tbspace_subxact SELECT -generate_series(1, 50); +ROLLBACK TO s; +COMMIT; +INSERT INTO tbspace_subxact SELECT generate_series(101, 300); +SELECT count(*) FROM tbspace_subxact WHERE a < 0; +SELECT count(*) FROM tbspace_subxact; +RESET enable_seqscan; +RESET enable_bitmapscan; +DROP TABLE tbspace_subxact; + ALTER TABLESPACE regress_tblspace RENAME TO regress_tblspace_renamed; ALTER TABLE ALL IN TABLESPACE regress_tblspace_renamed SET TABLESPACE pg_default; -- 2.55.0