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

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Konstantin Knizhnik <[email protected]>
To: Michael Paquier <[email protected]>
To: Imran Zaheer <[email protected]>
Cc: Srinath Reddy Sadipiralla <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
Date: Mon, 6 Jul 2026 16:38:14 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAFC+b6rLNvDzzYxrfLdq7CWbbouHcuOxi-mjEOW1r3qy2uAPMQ@mail.gmail.com>
	<CA+UBfan1QukfNDPvb2YpPG-ME4vUduBfbbe5uWptXDryZbtwVA@mail.gmail.com>
	<[email protected]>


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.

view thread (21+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox