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

On 06/07/2026 9:36 AM, Michael Paquier wrote:
> 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

I wonder if we also need to replace GetOldestNonRemovableTransactionId
with GetOldestNonRemovableTransactionIdForRewrite in  vacuum.c:

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index be863db81cb..aaa7e6eeeaa 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1149,7 +1149,7 @@ vacuum_get_cutoffs(Relation rel, const
VacuumParams *params,
         * that only one vacuum process can be working on a particular
table at
         * any time, and that each vacuum is always an independent
transaction.
         */
-       cutoffs->OldestXmin = GetOldestNonRemovableTransactionId(rel);
+       cutoffs->OldestXmin =
GetOldestNonRemovableTransactionIdForRewrite(rel);

        Assert(TransactionIdIsNormal(cutoffs->OldestXmin));

For some reasons the problem is not reproduced with current master, but
when I applied your patch to PG18 and run repro script with REPACK
replaced with VACUUM FULL, then I still get missing chunk error.