Message-ID: <0610480a-70e3-4a77-8bdf-94143fc3a4d7@garret.ru>
Date: Mon, 6 Jul 2026 16:38:14 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #19519: REPACK can fail due to missing chunk for toast value
To: Michael Paquier <michael@paquier.xyz>, Imran Zaheer <imran.zhir@gmail.com>
Cc: Srinath Reddy Sadipiralla <srinath2133@gmail.com>, exclusion@gmail.com,
 pgsql-bugs@lists.postgresql.org
References: <19519-fe02d8ff679d834d@postgresql.org>
 <CAFC+b6rLNvDzzYxrfLdq7CWbbouHcuOxi-mjEOW1r3qy2uAPMQ@mail.gmail.com>
 <CA+UBfan1QukfNDPvb2YpPG-ME4vUduBfbbe5uWptXDryZbtwVA@mail.gmail.com>
 <aktM919ANQGYZcsj@paquier.xyz>
Content-Language: en-US
From: Konstantin Knizhnik <knizhnik@garret.ru>
In-Reply-To: <aktM919ANQGYZcsj@paquier.xyz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/0610480a-70e3-4a77-8bdf-94143fc3a4d7%40garret.ru>
Precedence: bulk


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.