MIME-Version: 1.0
References: <19519-fe02d8ff679d834d@postgresql.org>
 <CAFC+b6rLNvDzzYxrfLdq7CWbbouHcuOxi-mjEOW1r3qy2uAPMQ@mail.gmail.com>
 <CA+UBfan1QukfNDPvb2YpPG-ME4vUduBfbbe5uWptXDryZbtwVA@mail.gmail.com>
 <aktM919ANQGYZcsj@paquier.xyz>
In-Reply-To: <aktM919ANQGYZcsj@paquier.xyz>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 6 Jul 2026 20:21:41 +0200
Message-ID: 
 <CAEze2WhCRX86ZY2QN7vuGc3wsAyOJbArFSc0i5KtMVnsguA7Kw@mail.gmail.com>
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
To: Michael Paquier <michael@paquier.xyz>
Cc: Imran Zaheer <imran.zhir@gmail.com>,
 Srinath Reddy Sadipiralla <srinath2133@gmail.com>, exclusion@gmail.com,
	pgsql-bugs@lists.postgresql.org, Konstantin Knizhnik <knizhnik@garret.ru>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAEze2WhCRX86ZY2QN7vuGc3wsAyOJbArFSc0i5KtMVnsguA7Kw%40mail.gmail.com>
Precedence: bulk

On Mon, 6 Jul 2026 at 18:40, Michael Paquier <michael@paquier.xyz> 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.

I don't think this approach actually fixes the underlying issue: As I
understand it; the problem is this:

1. OldestNonRemovableTransactionId() is based on the xmin state across
backends. It may increase at any point in time, could even decrease in
certain circumstances[0], and can be in an inconsistent state across
backends.

2. Data is only protected against concurrent cleanup while it's
visible to a MVCC snapshot in some backend, and this is advertised
through the xmin of the backend holding the snapshot being <= the xmin
of the snapshot.
   This relation-level cleanup horizon is determined with
GlobalVisTestFor(), which uses the same data source as
OldestNonRemovableTransactionId().

3. All relevant cases of REPACK and CREATE INDEX use SnapshotAny + a
cached output of OldestNonRemovableTransactionId()

The issue here is that CREATE INDEX and REPACK (CI/R) can start with
an OldestXmin of (say) 100, then another backend that held back that
OldestXmin stops holding back the xmin (allowing the output of a new
invocation of OldestNonRemovableTransactionId() to return e.g. 200),
after which the cleanup starts for tuples deleted with transaction ID
< 200, rather than the <100 expected by INDEX/REPACK.

Because both CI and R use SnapshotAny, and stored the result of
OldestNonRemovableTransactionId() from the start of their command
processing, they'll consider any still-present tuple >= 100 to be
visible for their scan.
However, every other backend has stopped considering these tuples
visible for their MVCC, scans and therefore may have started cleaning
up the tuple data. For main relation's heap pages, this cleanup isn't
(much) of an issue, because the fully dead and removed tuples would
have to be cleaned up later anyway and are not interesting for the
scan, but the related toast tuples can also be cleaned up without
first having to wait for the main tuple to be cleaned up, which is
exactly what this issue shows: TOAST expects to be used in an MVCC
context, and thus errors when it fails to find the tuples that were
reclaimed. It's a de-sync between what this backend and what other
backends expect.

So, in effect, this is an issue that's quite similar to the CIC/RIC
bug of PG14<14.4, except that
1.) in this case it's SnapshotAny instead of a
registered-but-invisible MVCC snapshot;
2.) the issue in this case only happens when toast pointers were
cleaned up and then are dereferenced, rather than HOT chains updated
and reclaimed since the scan started; and finally
3.) this issue dates back a very long time, likely since TOAST, but
defininitely since PG14 introduced the GlobalVisTestFor apis.


So I think the only way to fix this is either
1.) we update every maintenance process that makes use of SnapshotAny
and could access TOAST tables to handle errors caused by missing TOAST
data when the base relation's tuple is RECENTLY_DEAD, and treat those
as "tuple cleanup has already started, we just weren't aware of it
yet, so ignore this tuple" (as I mentioned in [1]), or
2.) we set the xmin of the CI/R backend to the current OldestXmin for
the relation(s) we're currently processing, and so block all cleanup
of the relevant (toast) data for the duration of this statement or
whenever we check for a newer xmin for the relation (whilst ignoring
our current one).


Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

[0]: "BUG #17257: (auto)vacuum hangs within lazy_scan_prune()"
https://www.postgresql.org/message-id/flat/CAH2-WznTo_nkbTAoBO2mCR5TzWBj8fEa4%252Bk-K3x9YUYZ0AKVUQ%2540mail.gmail.com
[1]: "Re: VACUUM FULL or CREATE INDEX fails with error: missing chunk
number 0 for toast value XXX" at -hackers:
https://www.postgresql.org/message-id/CAEze2WgRUfoR%3DtSQQm8zexVNeBbEYi9ZSPztrK63ObyfPxKpBw%40mail.gmail.com