Re: BUG #19519: REPACK can fail due to missing chunk for toast value

On Fri, Jul 03, 2026 at 11:45:42AM +0500, Imran Zaheer wrote:
> Can we safely use the same fix in the index build path too? Can we use
> GetOldestNonRemovableTransactionIdforRewrite or something similar
> here? Normal serial index builds use SnapShotAny and concurrent index
> builds use MVCC, but the bug only exists in the serial index build
> path.

Yeah, I think that we should be able to safely reuse your ForRewrite()
API for the index build case (SnapshotAny part) as well to make the
horizon computations more conservative, exclusing one own XID as we
do in the lazy VACUUM case.

> Other than that, after applying your patch, the bug was not
> reproducible with either this repro or the other report's repro [1] in
> the rewrite path. However, the create index bug is still there. You
> can use the following repro as mentioned in the thread [1].

The assymetry between the global xmin computation and the per-database
horizon is what's killing us here. All these patterns are complicated
enough that they warrant some tests, even if it is necessary to have
multiple databases to trigger the buggy horizon computations. TAP
tests would be an obvious choice for that. Now I have a set of tricks
in my sleeves to make an isolation test fully deterministic:
- dblink() that opens a transaction to a different database than the
one of the isolation regression database.
- Rename of a TOAST table using allow_system_table_mods, for a VACUUM.

The second one is one of the dirtiest tricks I've done in the past for
a REINDEX CONCURRENTLY test with TOAST. Quite useful. If the backend
side change is reverted, the tests fail.

The attached includes both test suites for all four cases reported
(REPACK, VACUUM, CLUSTER, non-MVCC index builds). We'll need to
remove one of them, just keeping both posted as I am not sure if the
isolation tests would be entirely stable in the buildfarm..

I still need to dive more into the code, but for now this bit stands
out:
vacuum_get_cutoffs(OldHeap, &params, &cutoffs);

+ /*
+ * vacuum_get_cutoffs() folds our own backend's xmin into OldestXmin. For
+ * a rewrite that is too conservative: the snapshot we hold exists only to
+ * evaluate index expressions against other relations, not to read
+ * OldHeap's historical rows. If our xmin is held back by a transaction
+ * that cannot even see OldHeap (e.g. one in another database), we would
+ * preserve a recently-dead tuple whose TOAST chunks a concurrent or prior
+ * lazy vacuum was free to remove, and then fail with "missing chunk" while
+ * copying it. Recompute OldestXmin ignoring our own backend so it matches
+ * the horizon lazy vacuum uses. This can only move OldestXmin forward, so
+ * the freeze cutoffs derived above remain valid.
+ */
+ cutoffs.OldestXmin = GetOldestNonRemovableTransactionIdForRewrite(OldHeap);

In copy_table_data() (for the data copy with repack, cluster, vacuum
full), I think that this is incorrect. For one, this breaks the
FreezeLimit which should always be older than OldestXmin, and this
patch enforces a new recomputation of OldestXmin ignoring most of the
internals of vacuum_get_cutoffs(). That's brittle, to say the least
if we change the way the vacuum cutoffs are calculated in the future.

Thoughts and comments from others are welcome for now. My day is
almost out, at least I got all these scenarios working some tests.
--
Michael