From: Manu <manuelreyesbravo@gmail.com>
To: Alexandre Felipe <o.alexandre.felipe@gmail.com>
To: pgsql-bugs@lists.postgresql.org
Cc: Alexander Lakhin <exclusion@gmail.com>
Subject: Re: BUG #19686: Rolling back SET TABLESPACE + INSERT leads to index corruption
Date: Sun, 27 Sep 2026 13:11:13 -0300
Message-ID: <179052547394.318321.10043901456417388246@gmail.com> (raw)
In-Reply-To: <CAE8JnxOE2rerDxuapP-14LcR+8HdD-8+1-PQ644VNQpsjkXK-w@mail.gmail.com>
References: <CAE8JnxOE2rerDxuapP-14LcR+8HdD-8+1-PQ644VNQpsjkXK-w@mail.gmail.com>
On 2026-09-15 08:59, Alexandre Felipe wrote:
> I started investigating.
> ...
> Anyone else looking into this?
I looked into it too; here is what I found, in case it is useful.
The trigger is that ALTER TABLE ... SET TABLESPACE gives the heap a new
relfilenode while the table's indexes deliberately keep theirs. The two
then unwind differently on abort: the heap's new file is discarded, so
the heap TIDs used by rows inserted after the SET TABLESPACE are free
again, but the index entries for those rows were written to the
unchanged index files and survive. A later insert reuses one of those
freed TIDs, and the index is left with two entries pointing at the same
live heap tuple.
That fits your point that it is not really an nbtree problem: gist shows
the same duplicate through an index-only scan, and the _bt_posting_valid
assert from 0d861bbb7 only makes btree notice it. Dropping the insert,
or the SET TABLESPACE, or turning the insert into an update all avoid
it, which lines up with the TID-reuse explanation.
Attached is a patch for discussion. After the heap is moved,
ATExecSetTableSpace gives each of the table's indexes a new
relfilenumber by copying it within its own tablespace, so the indexes
share the heap's rollback: on abort the new heap and index files are
discarded together, and on commit they are kept together. It closes
both the btree and the gist case here, and make check-world passes.
I first tried reindexing the indexes instead, which also fixes it, but
on a 1M-row table with three indexes that made SET TABLESPACE roughly
20x slower (about 100 ms to 1.9 s); copying the files keeps it near 2x.
As far as I can tell an abort-time fix is not possible, so SET TABLESPACE
has to make the indexes safe up front, but whether this is the right way
to do it is a question for people who know this code better than I do.
Manu
Attachments:
[text/x-patch] v1-0001-Fix-index-corruption-SET-TABLESPACE-rollback.patch (6.6K, ../179052547394.318321.10043901456417388246@gmail.com/2-v1-0001-Fix-index-corruption-SET-TABLESPACE-rollback.patch)
download | inline diff:
From 3c5662ff268ac2b2e803130b7199517160f5a4e9 Mon Sep 17 00:00:00 2001
From: Manu <manuelreyesbravo@gmail.com>
Date: Sun, 27 Sep 2026 13:08:36 -0300
Subject: [PATCH] Fix index corruption after rolling back ALTER TABLE SET
TABLESPACE
ALTER TABLE ... SET TABLESPACE rewrites a table's heap to a new
relfilenode but deliberately 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, so the heap TIDs consumed by
rows inserted after the SET TABLESPACE become free again, while the
index entries for those rows were written to the unchanged index files
and survive the abort. 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.
Make the indexes share the heap's rewrite: after moving the heap, give
each of the table's indexes a new relfilenumber by copying it within its
own tablespace, so that an abort discards the new heap and index files
together and a commit keeps them together.
Bug: #19686
Reported-by: Alexander Lakhin
---
src/backend/commands/tablecmds.c | 93 ++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 0274d892f2e..918d517ac23 100644--- a/src/backend/commands/tablecmds.c+++ b/src/backend/commands/tablecmds.c@@ -695,6 +695,7 @@ static void ATPrepChangePersistence(AlteredTableInfo *tab, Relation rel,
static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
const char *tablespacename, LOCKMODE lockmode);
static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
+static void ATExecSetTableSpaceNewIndexRelfilenumber(Oid indexOid, LOCKMODE lockmode);
static void ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace);
static void ATExecSetRelOptions(Relation rel, List *defList,
AlterTableType operation,
@@ -17520,9 +17521,11 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
{
Relation rel;
Oid reltoastrelid;
+ char relkind;
RelFileNumber newrelfilenumber;
RelFileLocator newrlocator;
List *reltoastidxids = NIL;
+ List *reltabidxids = NIL;
ListCell *lc;
/*
@@ -17540,6 +17543,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))
{
@@ -17586,6 +17590,13 @@ 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 (relkind == RELKIND_RELATION || relkind == RELKIND_MATVIEW)+ reltabidxids = RelationGetIndexList(rel);+
relation_close(rel, NoLock);
/* Make sure the reltablespace change is visible */
@@ -17599,6 +17610,88 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
/* Clean up */
list_free(reltoastidxids);
++ /*+ * Moving a table's heap assigns it a new relfilenode, but its indexes are+ * deliberately left in place with their existing relfilenodes. That mix+ * is unsafe across a rollback: if the transaction inserts into the table+ * after this and then aborts, the heap's new relfilenode is discarded and+ * its file reverts to the pre-move contents, freeing the TIDs used by the+ * aborted rows; but the matching index entries were written to the+ * unchanged index files and survive the abort. A later insert can reuse a+ * freed heap TID, leaving two index entries pointing at the same live heap+ * tuple -- index corruption (bug #19686). Give each index a fresh+ * relfilenumber, copied within its own tablespace, so it shares the heap's+ * new-relfilenode fate: on abort the new heap and index files are all+ * discarded together, and on commit they are all kept.+ */+ foreach(lc, reltabidxids)+ ATExecSetTableSpaceNewIndexRelfilenumber(lfirst_oid(lc), lockmode);+ list_free(reltabidxids);+}++/*+ * 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 that rewrite's+ * transactional fate, otherwise an abort discards the heap's new file (freeing+ * its TIDs) while leaving behind index entries that were written during the+ * transaction, which a later insert can then alias -- index corruption (bug+ * #19686). Unlike a full reindex this merely copies the existing index file,+ * so the added cost stays close to that of the heap move itself.+ */+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. */+ if (ind->rd_rel->relkind != RELKIND_INDEX ||+ !RELKIND_HAS_STORAGE(ind->rd_rel->relkind))+ {+ 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();
}
/*
--
2.55.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: manuelreyesbravo@gmail.com, o.alexandre.felipe@gmail.com, pgsql-bugs@lists.postgresql.org, exclusion@gmail.com
Subject: Re: BUG #19686: Rolling back SET TABLESPACE + INSERT leads to index corruption
In-Reply-To: <179052547394.318321.10043901456417388246@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox