Hi,
> I'm pretty doubtful that is a good idea. For one it'll make the performance
> effects very hard to understand.
> The outer query might have some buffers of the "old" index pinned etc
Agreed on both, so I've dropped the deferred copy. v5, attached, goes
back to copying the indexes in the ALTER, as v3 did, with the two
optimizations from your other mail, which Tom +1'd:
1) No copy when the ALTER is a top-level statement outside a transaction
block. AlterTableUtilityContext gets an isTopLevel field for that. Two
things can still write to the table in that transaction after the
ALTER, and v5 guards against both. A ddl_command_end event trigger: the
shortcut is off when one exists, which is cheap to check. A later
statement of an extended-protocol pipeline, which shares the transaction
until Sync: v5 sets XACT_FLAGS_NEEDIMMEDIATECOMMIT, as
PreventInTransactionBlock does, so a pipelined ALTER ... SET TABLESPACE
now commits on its own. With the guards removed, each case leaves 50
stale index entries; with v5, none.
2) No copy for an index whose current file was created in the current
subtransaction. It has to be the file's subtransaction, not the
transaction: an index created, reindexed or already copied before a
savepoint survives a rollback to it, and needs the copy. This also
covers your workaround for Tom's case: moving the indexes first and then
the table copies each index once. The regression test checks both.
For the back branches, isTopLevel goes at the end of
AlterTableUtilityContext; core is the only place I found that fills it.
make check passes on master, REL_15 and REL_14 (243, 217 and 216
tests). I also ran randomized transactions (nested savepoints, table
and index moves, REINDEX, DML, TRUNCATE, ROLLBACK TO, top-level moves),
comparing an index scan with a seq scan after each one: 40,000 with v5
and no mismatch, while the same test finds the bug on master. Results
are the same on a streaming standby, after an immediate-mode crash, and
with wal_level=minimal. The nested-query case can't come up now: the
ALTER refuses a table with an open scan ("being used by active
queries").
Regards,
Manu
From f818dfd16cd2121b8b85f919d03ea494dd43f6e0 Mon Sep 17 00:00:00 2001
From: Manuel Reyes Bravo <[email protected]>
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
From 12d170ec0f1ad3eed71030b205b9bd4d42c72644 Mon Sep 17 00:00:00 2001
From: Manuel Reyes Bravo <[email protected]>
Date: Fri, 2 Oct 2026 10:47:53 -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 3fd375feffc..ea4245406c3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -92,6 +92,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"
@@ -593,7 +594,10 @@ static void ATPrepSetAccessMethod(AlteredTableInfo *tab,
Relation rel, const cha
static bool ATPrepChangePersistence(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 ATExecSetTableSpaceNewIndexRelfilenode(Oid indexOid, LOCKMODE
lockmode);
static void ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace);
static void ATExecSetRelOptions(Relation rel, List *defList,
AlterTableType
operation,
@@ -5670,7 +5674,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));
}
/*
@@ -14791,13 +14796,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;
Oid newrelfilenode;
RelFileNode newrnode;
List *reltoastidxids = NIL;
+ List *reltabidxids = NIL;
ListCell *lc;
/*
@@ -14815,6 +14823,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))
{
@@ -14861,6 +14870,14 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace,
LOCKMODE lockmode)
RelationAssumeNewRelfilenode(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 relfilenode below.
+ */
+ if (copyIndexes &&
+ (relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW))
+ reltabidxids = RelationGetIndexList(rel);
+
relation_close(rel, NoLock);
/* Make sure the reltablespace change is visible */
@@ -14868,12 +14885,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 ATExecSetTableSpaceNewIndexRelfilenode.
+ */
+ foreach(lc, reltabidxids)
+ ATExecSetTableSpaceNewIndexRelfilenode(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 relfilenode within its existing
+ * tablespace, copying the current index file to the new relfilenode.
+ *
+ * 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
+ATExecSetTableSpaceNewIndexRelfilenode(Oid indexOid, LOCKMODE lockmode)
+{
+ Relation ind;
+ Oid newrelfilenode;
+ RelFileNode newrnode;
+ 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_newRelfilenodeSubid 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_newRelfilenodeSubid, ind->rd_createSubid) ==
+ GetCurrentSubTransactionId())
+ {
+ relation_close(ind, NoLock);
+ return;
+ }
+
+ /* Allocate a new relfilenode in the index's current tablespace. */
+ newrelfilenode = GetNewRelFileNode(ind->rd_rel->reltablespace, NULL,
+
ind->rd_rel->relpersistence);
+ newrnode = ind->rd_node;
+ newrnode.relNode = newrelfilenode;
+
+ /* Copy the index into the new file and schedule the old one for
cleanup. */
+ index_copy_data(ind, newrnode);
+
+ /* 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 = newrelfilenode;
+ CatalogTupleUpdate(pg_class, &otid, tuple);
+ UnlockTuple(pg_class, &otid, InplaceUpdateTupleLock);
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+
+ InvokeObjectPostAlterHook(RelationRelationId, indexOid, 0);
+ RelationAssumeNewRelfilenode(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 dbbe5904256..478fb5c0071 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1316,6 +1316,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 f9daf5b744c..3123d4aa8dd 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 3873e3cd7fc..76899112108 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 060698072e6..9cfd4632ec1 100644
--- a/src/test/regress/sql/tablespace.sql
+++ b/src/test/regress/sql/tablespace.sql
@@ -422,6 +422,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
From 681ba2f13cdbda815b2ad425ca0e919b9085ad98 Mon Sep 17 00:00:00 2001
From: Manuel Reyes Bravo <[email protected]>
Date: Fri, 2 Oct 2026 10:50:40 -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/input/tablespace.source | 76 +++++++++++++
src/test/regress/output/tablespace.source | 120 ++++++++++++++++++++
5 files changed, 325 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e445b801a6b..12ae8916d26 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -89,6 +89,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"
@@ -549,7 +550,10 @@ static void ATExecDropCluster(Relation rel, LOCKMODE
lockmode);
static bool ATPrepChangePersistence(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 ATExecSetTableSpaceNewIndexRelfilenode(Oid indexOid, LOCKMODE
lockmode);
static void ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace);
static void ATExecSetRelOptions(Relation rel, List *defList,
AlterTableType
operation,
@@ -5582,7 +5586,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));
}
}
@@ -14198,13 +14203,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;
Oid newrelfilenode;
RelFileNode newrnode;
List *reltoastidxids = NIL;
+ List *reltabidxids = NIL;
ListCell *lc;
/*
@@ -14222,6 +14230,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))
{
@@ -14270,6 +14279,14 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace,
LOCKMODE lockmode)
RelationAssumeNewRelfilenode(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 relfilenode below.
+ */
+ if (copyIndexes &&
+ (relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW))
+ reltabidxids = RelationGetIndexList(rel);
+
relation_close(rel, NoLock);
/* Make sure the reltablespace change is visible */
@@ -14277,12 +14294,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 ATExecSetTableSpaceNewIndexRelfilenode.
+ */
+ foreach(lc, reltabidxids)
+ ATExecSetTableSpaceNewIndexRelfilenode(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 relfilenode within its existing
+ * tablespace, copying the current index file to the new relfilenode.
+ *
+ * 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
+ATExecSetTableSpaceNewIndexRelfilenode(Oid indexOid, LOCKMODE lockmode)
+{
+ Relation ind;
+ Oid newrelfilenode;
+ RelFileNode newrnode;
+ 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_newRelfilenodeSubid 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_newRelfilenodeSubid, ind->rd_createSubid) ==
+ GetCurrentSubTransactionId())
+ {
+ relation_close(ind, NoLock);
+ return;
+ }
+
+ /* Allocate a new relfilenode in the index's current tablespace. */
+ newrelfilenode = GetNewRelFileNode(ind->rd_rel->reltablespace, NULL,
+
ind->rd_rel->relpersistence);
+ newrnode = ind->rd_node;
+ newrnode.relNode = newrelfilenode;
+
+ /* Copy the index into the new file and schedule the old one for
cleanup. */
+ index_copy_data(ind, newrnode);
+
+ /* 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 = newrelfilenode;
+ CatalogTupleUpdate(pg_class, &otid, tuple);
+ UnlockTuple(pg_class, &otid, InplaceUpdateTupleLock);
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+
+ InvokeObjectPostAlterHook(RelationRelationId, indexOid, 0);
+ RelationAssumeNewRelfilenode(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 51509efbfc1..cbfd800ad35 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1308,6 +1308,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 212e9b32806..7cd9a891d2b 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/input/tablespace.source
b/src/test/regress/input/tablespace.source
index fd003d805e7..783c8f9ecf3 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -400,6 +400,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;
diff --git a/src/test/regress/output/tablespace.source
b/src/test/regress/output/tablespace.source
index 1b60b99ff3e..a453035ec45 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -921,6 +921,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;
--
2.55.0
