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

Hi Alexander,

On Sun, Jun 14, 2026 at 10:37 PM PG Bug reporting form <
noreply@postgresql.org> wrote:

The following bug has been logged on the website:

The following script:
createdb db1
createdb db2

cat << EOF | psql db1
SET default_statistics_target = 1000;
CREATE TABLE t(i int, t text);
INSERT INTO t SELECT 1, g::text FROM generate_series(1, 50000) g;
ANALYZE t;
EOF

cat << EOF | psql db2 &
BEGIN;
CREATE TABLE t(i int);
SELECT pg_sleep(3);
EOF

sleep 1

cat << EOF | psql db1
DROP TABLE t;
VACUUM pg_toast.pg_toast_2619;
REPACK;
EOF
wait

triggers:
ERROR: missing chunk number 0 for toast value 16393 in pg_toast_2619

Thanks for the report and clean reproducer , I can also reproduce this.

I looked into this. It is not REPACK-specific, it reproduces with
plain VACUUM FULL on back branches too, so it predates
the REPACK command. AFAIU The root cause is a disagreement about
the removal horizon between a table rewrite and an independent vacuum
of that table's TOAST relation.

A rewrite (cluster_rel -> copy_table_data, copy_heap_data) preserves live
and RECENTLY_DEAD tuples, and detoasts a preserved tuple's external
values to move them into the new TOAST table.Its removal cutoff comes
from GetOldestNonRemovableTransactionId(), i.e. ComputeXidHorizons(),
which includes the rewriting backend's own snapshot xmin.
The rewrite holds an ordinary MVCC snapshot (taken so index expressions
can be evaluated) and deliberately does not set PROC_IN_VACUUM. So
The rewrite backend's snapshot xmin is the cluster-wide oldest
running xid, which the db2 transaction pins. Since that backend is
in db1 and lacks PROC_IN_VACUUM, its own xmin folds into the
relation's data/catalog horizon -> OldestXmin sits behind the
DROP, so the dead pg_statistic tuple is RECENTLY_DEAD and gets
copied.
The lazy vacuum of pg_toast_2619 sets PROC_IN_VACUUM, which excludes
its own xmin from the horizon, and the db2 transaction, being in another
database,
does not constrain a non-shared relation's horizon. Its cutoff advances
past the DROP
and the chunks are removed.The two operations thus disagree about the same
tuple,
and the rewrite ends up fetching TOAST the lazy vacuum already reclaimed.

Attached Fix:
The rewrite is simply too conservative,AFAIK it's snapshot is only there to
evaluate index expressions against other relations, and it is not a
reader of the rewritten relation's historical rows, so its own xmin
should not hold back that relation's removal horizon. Excluding it
yields exactly the horizon the lazy vacuum uses.
We cannot set PROC_IN_VACUUM on the rewrite, that would broadcast
"ignore my xmin" to every backend and let other vacuums remove tuples in
other relations that the index expressions may need (the documented
reason VACUUM FULL does not set it). The exclusion has to be local to
the rewrite's own horizon computation, so the patch adds
GetOldestNonRemovableTransactionIdForRewrite(): ComputeXidHorizons()
gains a flag to skip the calling backend (and to not publish the
resulting, more-aggressive horizons into the shared GlobalVisState), and
copy_table_data() uses it for OldestXmin.

--
Thanks :)
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

Hi,

The proposed fix (skipping own xmin while computing oldest xmin) did
solve the problem, but while testing this, I found another bug report
showing the same error: "missing chunks."
For example, this report [1]/messages/by-id/18351-f6e06364b3a2e669@postgresql.org reproduces the same bug with a different
repro attached, but interestingly, this thread discussion also pointed
out that the bug is also reproducible with the CREATE INDEX command.

The following trace was reported by Alexander Lakhin there.

The backtrace of the heap_fetch_toast_slice() call emitting error is:
2024-03-05 17:41:07.786 UTC|law|db10|65e75933.208c6b|ERROR: missing chunk number 0 for toast value 17314 in pg_toast_17289
2024-03-05 17:41:07.786 UTC|law|db10|65e75933.208c6b|BACKTRACE:
heap_fetch_toast_slice at heaptoast.c:784:3
toast_fetch_datum at detoast.c:379:2
detoast_external_attr at detoast.c:54:12
index_form_tuple_context at indextuple.c:111:21
tuplesort_putindextuplevalues at tuplesortvariants.c:678:15
_bt_spool at nbtsort.c:537:1
_bt_build_callback at nbtsort.c:610:24
heapam_index_build_range_scan at heapam_handler.c:1329:22
table_index_build_scan at tableam.h:1781:9
(inlined by) _bt_spools_heapscan at nbtsort.c:483:15
btbuild at nbtsort.c:329:14
index_build at index.c:3042:10
index_create at index.c:1265:3
DefineIndex at indexcmds.c:1166:3

Index building, together with lazy vacuum, can create similar
conditions as we saw with VACUUM FULL / CLUSTER / REPACK. In
heapam_index_build_range_scan(), we compute the OldestXmin, which
later is used to decide which tuples to index (in repack we use it to
decide which tuples to copy). I think we should also consider fixing
the index building path while addressing this bug.

```
OldestXmin = GetOldestNonRemovableTransactionId(heapRelation);
```

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.

Other than that, after applying your patch, the bug was not
reproducible with either this repro or the other report's repro [1]/messages/by-id/18351-f6e06364b3a2e669@postgresql.org in
the rewrite path. However, the create index bug is still there. You
can use the following repro as mentioned in the thread [1]/messages/by-id/18351-f6e06364b3a2e669@postgresql.org.

I've discovered that not only VACUUM FULL can stumble over such missing
toast values. CREATE INDEX behaves similarly, as the following script
shows:
for ((c=1;c<=20;c++)); do createdb db$c; done

for ((i=1;i<=100;i++)); do
echo "iteration $i"
for ((c=1;c<=20;c++)); do
echo "\\d sometable" | psql -d db$c >psql-1-$c.log 2>&1 &
echo "DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl (i int, t text);
ALTER TABLE tbl ALTER COLUMN t SET STORAGE EXTERNAL;
INSERT INTO tbl(i, t) VALUES (1, repeat('1234567890', 250));
DELETE FROM tbl;

VACUUM (VERBOSE) tbl;
CREATE INDEX t_idx ON tbl(t);
" | psql -d db$c >psql-2-$c.log 2>&1 &
done
wait
grep 'missing chunk number' server.log && break;
done

Regards,
Imran Zaheer

[1]: /messages/by-id/18351-f6e06364b3a2e669@postgresql.org

On Wed, Jun 17, 2026 at 10:03 PM Srinath Reddy Sadipiralla
<srinath2133@gmail.com> 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

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.

Hello Michael,

06.07.2026 09:36, Michael Paquier wrote:

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

Unfortunately, I still can produce that error running my repro script,
with higher concurrency:
for ((c=1;c<=50;c++)); do createdb db$c; done

for ((i=1;i<=100;i++)); do
echo "iteration $i"
  for ((c=1;c<=50;c++)); do
echo "\\d sometable" | psql -d db$c >psql-1-$c.log 2>&1 &
echo "DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl (i int, t text);
ALTER TABLE tbl ALTER COLUMN t SET STORAGE EXTERNAL;
INSERT INTO tbl(i, t) VALUES (1, repeat('1234567890', 250));
DELETE FROM tbl;

VACUUM (TRUNCATE, VERBOSE) tbl;
VACUUM FULL tbl;
" | psql -d db$c >psql-$c.log 2>&1 &
  done
wait
grep 'missing chunk number' server.log  && break;
done

...
iteration 45
iteration 46
iteration 47
2026-07-06 17:38:19.952 UTC|law|db22|6a4be80b.3ab9a8|ERROR:  missing chunk number 0 for toast value 41866 in pg_toast_41849

I had failures on iterations 47, 58, 79.

(Konstantin's patch doesn't help either.)

Best regards,
Alexander

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]"BUG #17257: (auto)vacuum hangs within lazy_scan_prune()" /messages/by-id/CAH2-WznTo_nkbTAoBO2mCR5TzWBj8fEa4+k-K3x9YUYZ0AKVUQ@mail.gmail.com, 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]"Re: VACUUM FULL or CREATE INDEX fails with error: missing chunk number 0 for toast value XXX" at -hackers: /messages/by-id/CAEze2WgRUfoR=tSQQm8zexVNeBbEYi9ZSPztrK63ObyfPxKpBw@mail.gmail.com), 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()" /messages/by-id/CAH2-WznTo_nkbTAoBO2mCR5TzWBj8fEa4+k-K3x9YUYZ0AKVUQ@mail.gmail.com
/messages/by-id/CAH2-WznTo_nkbTAoBO2mCR5TzWBj8fEa4+k-K3x9YUYZ0AKVUQ@mail.gmail.com
[1]: "Re: VACUUM FULL or CREATE INDEX fails with error: missing chunk number 0 for toast value XXX" at -hackers: /messages/by-id/CAEze2WgRUfoR=tSQQm8zexVNeBbEYi9ZSPztrK63ObyfPxKpBw@mail.gmail.com
number 0 for toast value XXX" at -hackers:
/messages/by-id/CAEze2WgRUfoR=tSQQm8zexVNeBbEYi9ZSPztrK63ObyfPxKpBw@mail.gmail.com

On Mon, Jul 06, 2026 at 08:21:41PM +0200, Matthias van de Meent wrote:

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.

You mean 042b584c7f7d, revert of d9d076222f5b. That rings a bell.

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

That seems like an option worth exploring to me, but I doubt that
we'll be able to get something that can be backpatched due to how
invasive it is. I strongly suspect that it would require at least one
table AM change to cope with the fact that we want to let the callers
be OK with TOAST chunks missing in some contexts when grabbing a toast
slice. That's perhaps for the best if we think about potential
regressions, this is scary and old enough that we may still be OK by
living without a backpatch.

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).

Err, I don't think that would be safe anyway? It's rather uncommon in
practice in schemas, but we could trigger function calls, like index
expressions, domains, whatever, that access data of other tables while
processing one table with a SnapshotAny. The CIC revert of 14.4 was
about such cases.
--
Michael

On Tue, 7 Jul 2026, 01:19 Michael Paquier, <michael@paquier.xyz> wrote:

On Mon, Jul 06, 2026 at 08:21:41PM +0200, Matthias van de Meent wrote:

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.

You mean 042b584c7f7d, revert of d9d076222f5b. That rings a bell.

Indeed

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

That seems like an option worth exploring to me, but I doubt that
we'll be able to get something that can be backpatched due to how
invasive it is. I strongly suspect that it would require at least one
table AM change to cope with the fact that we want to let the callers
be OK with TOAST chunks missing in some contexts when grabbing a toast
slice. That's perhaps for the best if we think about potential
regressions, this is scary and old enough that we may still be OK by
living without a backpatch.

I suspect that for indexing we're lucky, in that we have full control
internally over which tuple data get fed into IndexBuildCallback. We can
make sure only fully detoasted attributes get passed on, and can be
implemented with some (a lot) of TRY/CATCH work in or around
FormIndexDatum. That said, I don't think that's very nice, and I'm also a
bit concerned about leaking things (memory, buffers, ...) in such an
approach.

For REPACK/VACUUM FULL/CLUSTER, I think we can do something similar, given
that this code too has a central loop processing the tuple data, though I
suspect it'll need a bit more work than the reindex one.

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).

Err, I don't think that would be safe anyway? It's rather uncommon in
practice in schemas, but we could trigger function calls, like index
expressions, domains, whatever, that access data of other tables while
processing one table with a SnapshotAny. The CIC revert of 14.4 was
about such cases.

True, but in indexes that is forbidden: mislabeling of non-INMUTABLE
functions causes broken indexes every day, and we can't be expected to make
it work there. Similarly, for REPACK it shouldn't evaluate any
non-indexable expressions, right?

Either way, if we can register a snapshot in the REPACK/INDEX backend whose
xmin is (*correctly*) registered with the OldestNonRemovableTransactionId()
of the relation, then concurrent cleanup won't happen for toast, and we'd
also avoid dereferencing those dangling toast pointers. We won't need to
actually use that snapshot for any scans; it'll avoid the issue as long as
our backend advertises an xmin <= the OldestXmin used in maintenance to
hold back pruning/vacuum on the relation.

(we can adjust that xmin every once in a while with a newer
OldestNonRemovableTransactionId (excl. our own backend) as long as we also
adjust the OldestXmin cutoff for the SnapshotAny visibility tests, but I
don't suggest to backpatch that even newer system, it'd add significant
complications to an already complicated system)

- Matthias

ps. sorry for the formatting, I'm writing this on my phone.

On Tue, Jul 07, 2026 at 01:51:32AM +0200, Matthias van de Meent wrote:

On Tue, 7 Jul 2026, 01:19 Michael Paquier, <michael@paquier.xyz> wrote:

On Mon, Jul 06, 2026 at 08:21:41PM +0200, Matthias van de Meent wrote:

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.

You mean 042b584c7f7d, revert of d9d076222f5b. That rings a bell.

Indeed

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

That seems like an option worth exploring to me, but I doubt that
we'll be able to get something that can be backpatched due to how
invasive it is. I strongly suspect that it would require at least one
table AM change to cope with the fact that we want to let the callers
be OK with TOAST chunks missing in some contexts when grabbing a toast
slice. That's perhaps for the best if we think about potential
regressions, this is scary and old enough that we may still be OK by
living without a backpatch.

I suspect that for indexing we're lucky, in that we have full control
internally over which tuple data get fed into IndexBuildCallback. We can
make sure only fully detoasted attributes get passed on, and can be
implemented with some (a lot) of TRY/CATCH work in or around
FormIndexDatum. That said, I don't think that's very nice, and I'm also a
bit concerned about leaking things (memory, buffers, ...) in such an
approach.

I've quickly thought about some try/catch blocks while detoasting, but
I cannot really get how this would be entirely bullet-proof. It seems
to me that we'll live better if we bite the bullet and go for the
invasive change of passing a state across the stack to let detoasting
know that we are dealing with a rewrite.

For REPACK/VACUUM FULL/CLUSTER, I think we can do something similar, given
that this code too has a central loop processing the tuple data, though I
suspect it'll need a bit more work than the reindex one.

While looking at the whole stack, the handling of dead tuples seems to
go through only index_build_range_scan() and
relation_copy_for_cluster() for heapam. A re-check for TOAST values
would be really bad if we have a lot of dead tuples.

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).

Err, I don't think that would be safe anyway? It's rather uncommon in
practice in schemas, but we could trigger function calls, like index
expressions, domains, whatever, that access data of other tables while
processing one table with a SnapshotAny. The CIC revert of 14.4 was
about such cases.

True, but in indexes that is forbidden: mislabeling of non-INMUTABLE
functions causes broken indexes every day, and we can't be expected to make
it work there. Similarly, for REPACK it shouldn't evaluate any
non-indexable expressions, right?

Still I am pretty sure that the window would exist, users like fancy
definitions. It seems at least worth looking if we could make
something more stable across the board. Just playing with it now, and
it does not seem that bad in practice, actually..

Either way, if we can register a snapshot in the REPACK/INDEX backend whose
xmin is (*correctly*) registered with the OldestNonRemovableTransactionId()
of the relation, then concurrent cleanup won't happen for toast, and we'd
also avoid dereferencing those dangling toast pointers. We won't need to
actually use that snapshot for any scans; it'll avoid the issue as long as
our backend advertises an xmin <= the OldestXmin used in maintenance to
hold back pruning/vacuum on the relation.

(we can adjust that xmin every once in a while with a newer
OldestNonRemovableTransactionId (excl. our own backend) as long as we also
adjust the OldestXmin cutoff for the SnapshotAny visibility tests, but I
don't suggest to backpatch that even newer system, it'd add significant
complications to an already complicated system)

I doubt that anything would be backpatchable, based on how the
discussion is going. I'm OK to be proven wrong.

ps. sorry for the formatting, I'm writing this on my phone.

No pb. Thanks.
--
Michael

On Tue, Jul 07, 2026 at 10:42:33AM +0900, Michael Paquier wrote:

I've quickly thought about some try/catch blocks while detoasting, but
I cannot really get how this would be entirely bullet-proof. It seems
to me that we'll live better if we bite the bullet and go for the
invasive change of passing a state across the stack to let detoasting
know that we are dealing with a rewrite.

I have been playing with this idea, with two approaches. For the
REPACK and VACUUM cases, things looked pretty straight for both
approaches, but the index build got hairy in the first case:
1) Attempt to shortcut missing TOAST chunks when doing index builds.
For hash and btree, things get straight with index_form_tuple(). But
I've quickly faced a wall with GIN (extractValue support function) and
GiST (compress function). So at the end I gave up on that, as it
would require more facilities than this looks worth for.
2) A more aggressive pre-detoast when building a range of values for
tuples that we are seeing as already dead, which should be able to
work across all index AMs transparently when we are dealing with dead
tuples (that should be kept in a per-tuple context). That's what I
guess you've mentioned upthread.

Option 2 leads to a much nicer result overall, with the attached
passing my regression tests and Alexander's case as well (highly
concurrent thing sent yesterday). The rest of the patch is kind of
boring, where I have been trying to get a missing_ok state across the
stack to let the toast slice fetch bypass the case of missing chunks
when we are OK with it due to the rewrites. That still feels crude,
but the simplicity is appealing here, as much as the rather low
invasiveness.

Thoughts or comments?
--
Michael

Hi,

On Tue, Jul 7, 2026 at 10:52 AM Michael Paquier <michael@paquier.xyz> wrote:

Option 2 leads to a much nicer result overall, with the attached
passing my regression tests and Alexander's case as well (highly
concurrent thing sent yesterday). The rest of the patch is kind of
boring, where I have been trying to get a missing_ok state across the
stack to let the toast slice fetch bypass the case of missing chunks
when we are OK with it due to the rewrites. That still feels crude,
but the simplicity is appealing here, as much as the rather low
invasiveness.

Thoughts or comments?

With the initial reproducer, I originally thought this bug was driven
entirely
by a long-running query in another database causing a self-inflicted horizon
drag on the rewrite operation. My theory was that the cross-database sleeper
drags down the global xmin, which infects the snapshot held by VACUUM FULL.
During ComputeXidHorizons, VACUUM FULL ends up dragging its own garbage
collection horizon backward. Lazy vacuum avoids this trap because it
utilizes the
PROC_IN_VACUUM flag to ignore its own snapshot.Since we cannot simply apply
PROC_IN_VACUUM to VACUUM FULL (it needs standard MVCC snapshots to
safely evaluate index expressions), and we can't remove it from lazy vacuum
(which would cause bloat), patching the rewrite operation to explicitly
ignore its
own snapshot during GC calculation (excludeMyself) seemed like the correct
fix.

The script proved that OldestXmin can be dragged backward by other
processes,
bypassing the patch entirely.If a completely separate process in the same
database
(such as a simple \d catalog lookup) requests a global snapshot, it gets
"infected" by
the long-running query in the other database. This new query now sits in
the local ProcArray.
Lazy Vacuum runs, ignores the cross-database sleeper, calculates an
aggressive horizon,
and physically removes the TOAST chunks.VACUUM FULL runs, sees the local
carrier
query, and is forced to respect its older snapshot. Its horizon is dragged
backward, it
assumes the main tuples are RECENTLY_DEAD, attempts to copy them, and
crashes
because the TOAST chunks are gone.

so my initial thoughts are that until there's dependency of
OldestXmin calculation on
global snapshot , the rewrite horizon getting dragged back seems
inevitable, so i think
compared to other approaches in the thread ,your approach of ignoring the
fact of
missing chunks seems good. I quickly reviewed the core logic of treating
the missing
chunks as dead and LGTM will further look into this and test.

--
Thanks :)
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/

On Tue, Jul 07, 2026 at 10:40:12PM +0530, Srinath Reddy Sadipiralla wrote:

I quickly reviewed the core logic of treating the missing
chunks as dead and LGTM will further look into this and test.

Thanks for that. This is tricky and invasive enough that this
warrants more eyes.

One thing that I still don't like much in the patch as written is my
use of a missing_ok argument, which feels super confusing as it
applies only to the underlying external TOAST, if the relation has
any. I'd be tempted to rewrite this portion of the patch with a
uint32 flags. Even if we assign one value for now (say MISSING_TOAST,
MISSING_TOAST_OK or whatever), it would allow more flexibility on ABI
grounds if we want more like states in the future across this portion
of the stack. Again, I strongly doubt that we will be able to
backpatch any of that.
--
Michael

On Wed, Jul 08, 2026 at 07:15:16AM +0900, Michael Paquier wrote:

One thing that I still don't like much in the patch as written is my
use of a missing_ok argument, which feels super confusing as it
applies only to the underlying external TOAST, if the relation has
any. I'd be tempted to rewrite this portion of the patch with a
uint32 flags. Even if we assign one value for now (say MISSING_TOAST,
MISSING_TOAST_OK or whatever), it would allow more flexibility on ABI
grounds if we want more like states in the future across this portion
of the stack. Again, I strongly doubt that we will be able to
backpatch any of that.

A couple of extra notes while I do not forget about that stuff.. The
patch may be better split into two if we go with this approach, as the
index build and rewrite paths require different solutions:
- One for the rewrite path. It makes little sense to do any kind of
aggressive early detoasting because it may be wasteful due to the
tuple rewrites that update their data with only copies by reference
(main relation tuple is rewritten, reuses the same external TOAST
tuple). For workloads where UPDATEs do not touch the TOASTed
attributes, that would be a waste.
- One for the index build path, which is actually too aggressive with
its early detoasting, now that I think about it. There should be no
need to perform a detoast for anything else than the attributes that
are used in the index definition or the attributes that are used in
index expressions. So as written this patch would lead to a
regression.
--
Michael

On 08/07/2026 04:10, Michael Paquier wrote:

On Wed, Jul 08, 2026 at 07:15:16AM +0900, Michael Paquier wrote:

One thing that I still don't like much in the patch as written is my
use of a missing_ok argument, which feels super confusing as it
applies only to the underlying external TOAST, if the relation has
any. I'd be tempted to rewrite this portion of the patch with a
uint32 flags. Even if we assign one value for now (say MISSING_TOAST,
MISSING_TOAST_OK or whatever), it would allow more flexibility on ABI
grounds if we want more like states in the future across this portion
of the stack. Again, I strongly doubt that we will be able to
backpatch any of that.

Yeah, boolean arguments in general can be confusing. Especially
arguments like "missing_ok" where it's hard to remember which behavior
is true and which is false. An IDE that shows the name of the argument
helps, but having the caller say "MISSING_TOAST_OK" rather than "true"
is much clearer.

For backpatching, I think we could smuggle the flag in a global
variable, to avoid changing the signature of the
relation_fetch_toast_slice() callback. See attached patch 0002 on top of
your 0001 patch (which is also attached for completeness). It doesn't
fix the problem for hypothetical 3rd party tableam implementations that
implement their own relation_fetch_toast_slice() callback. But do such
extensions even exist? If yes, they could be fixed too by also checking
the global variable.

I thought about adding an extra "extended" callback next to
relation_fetch_toast_slice(), but I don't see a way to add functions to
TableAmRoutine in an ABI-compatible way. For the future, we might want
to store sizeof(TableAmRoutine) in the struct itself, so that we could
add fields to it in minor versions without breaking the API, in case we
need something like this again.

A couple of extra notes while I do not forget about that stuff.. The
patch may be better split into two if we go with this approach, as the
index build and rewrite paths require different solutions:
- One for the rewrite path. It makes little sense to do any kind of
aggressive early detoasting because it may be wasteful due to the
tuple rewrites that update their data with only copies by reference
(main relation tuple is rewritten, reuses the same external TOAST
tuple). For workloads where UPDATEs do not touch the TOASTed
attributes, that would be a waste.

I didn't understand this part. Do you mean when rebuilding the indexes
after rewriting the heap? I didn't think we support storing toasted
external datums in an index at all.

- One for the index build path, which is actually too aggressive with
its early detoasting, now that I think about it. There should be no
need to perform a detoast for anything else than the attributes that
are used in the index definition or the attributes that are used in
index expressions. So as written this patch would lead to a
regression.

Yeah, although I wouldn't be too worried about the performance here. The
penalty would be when building an index on values that are large enough
to be toasted, with lots of RECENTLY_DEAD tuples. That doesn't seem like
a very common case.

- Heikki

Hi.

@@ -934,10 +953,17 @@ heapam_relation_copy_for_cluster(Relation
OldHeap, Relation NewHeap,

-                reform_and_rewrite_tuple(tuple,
-                                         OldHeap, NewHeap,
-                                         values, isnull,
-                                         rwstate);
+            {
+                if (!reform_and_rewrite_tuple(tuple,
+                                              OldHeap, NewHeap,
+                                              values, isnull,
+                                              rwstate, true))
+                {
+                    /* TOAST gone for a recently dead tuple */
+                    n_tuples -= 1;
+                    continue;
+                }
+            }

The tuplesort_getheaptuple() will contain both live and recently
deleted tuples. However, the proposed fix passes missing_ok=true for
all tuples coming out of the sort, which means a toast fetch failure
for a LIVE tuple would be acceptable and will be silently skipped. Or
am I missing something?

Thanks
Imran Zaheer

On Wed, Jul 08, 2026 at 08:00:28PM +0500, Imran Zaheer wrote:

The tuplesort_getheaptuple() will contain both live and recently
deleted tuples. However, the proposed fix passes missing_ok=true for
all tuples coming out of the sort, which means a toast fetch failure
for a LIVE tuple would be acceptable and will be silently skipped. Or
am I missing something?

Argh, thanks. Using missing_ok=true for the sort path is broken. The
flag should be false, instead. When dealing with the sort of the
tuples, we would already have made sure that the tuples with missing
toast chunks have been discarded, so a plain error with a missing
chunk would point to an invalid case.

I'm taking some time today to rework the patch set. Will add this
adjustment in it.
--
Michael

On Wed, Jul 08, 2026 at 03:00:33PM +0300, Heikki Linnakangas wrote:

On 08/07/2026 04:10, Michael Paquier wrote:
Yeah, boolean arguments in general can be confusing. Especially arguments
like "missing_ok" where it's hard to remember which behavior is true and
which is false. An IDE that shows the name of the argument helps, but having
the caller say "MISSING_TOAST_OK" rather than "true" is much clearer.

For backpatching, I think we could smuggle the flag in a global variable, to
avoid changing the signature of the relation_fetch_toast_slice() callback.
See attached patch 0002 on top of your 0001 patch (which is also attached
for completeness). It doesn't fix the problem for hypothetical 3rd party
tableam implementations that implement their own
relation_fetch_toast_slice() callback. But do such extensions even exist? If
yes, they could be fixed too by also checking the global variable.

One of my issues with that is that the static states can go out of
sync very easily. I'd like to think we don't have that many
extensions, but it's really hard to be sure. Silent breakages across
minor releases are never cool.

I thought about adding an extra "extended" callback next to
relation_fetch_toast_slice(), but I don't see a way to add functions to
TableAmRoutine in an ABI-compatible way. For the future, we might want to
store sizeof(TableAmRoutine) in the struct itself, so that we could add
fields to it in minor versions without breaking the API, in case we need
something like this again.

Right, I didn't consider this one.

- One for the rewrite path. It makes little sense to do any kind of
aggressive early detoasting because it may be wasteful due to the
tuple rewrites that update their data with only copies by reference
(main relation tuple is rewritten, reuses the same external TOAST
tuple). For workloads where UPDATEs do not touch the TOASTed
attributes, that would be a waste.

I didn't understand this part. Do you mean when rebuilding the indexes after
rewriting the heap? I didn't think we support storing toasted external
datums in an index at all.

I was referring to toast_save_datum() (touched this area a few weeks
ago for the 8-byte TOAST thing), where we touch only the
varatt_external on the main relation in some rewrite case. Forcing a
detoast for all tuples in such cases would make the rewrite less
efficient, no?

- One for the index build path, which is actually too aggressive with
its early detoasting, now that I think about it. There should be no
need to perform a detoast for anything else than the attributes that
are used in the index definition or the attributes that are used in
index expressions. So as written this patch would lead to a
regression.

Yeah, although I wouldn't be too worried about the performance here. The
penalty would be when building an index on values that are large enough to
be toasted, with lots of RECENTLY_DEAD tuples. That doesn't seem like a very
common case.

Pulling the varnos from the index predicate and expressions makes that
possible, thanks to ii_NumIndexAttrs.

An updated patch is attached, with the "flags" business added, and
something for the other issue reported by Imran for the case where
reform_and_rewrite_tuple() was not handled correctly. If we have a
sort, that means going back to a more aggressive detoasting to check
if some chunks are missing if a tuple has been found as recently dead.
I was wondering about doing a split into two patches for
build_range_scan() and copy_for_cluster(), but refrained from that due
to the detoast_external_attr_extended() required in both cases.

Anyway, thoughts and reviews are welcome.
--
Michael

On 08/07/2026 3:00 PM, Heikki Linnakangas wrote:

On 08/07/2026 04:10, Michael Paquier wrote:

On Wed, Jul 08, 2026 at 07:15:16AM +0900, Michael Paquier wrote:

One thing that I still don't like much in the patch as written is my
use of a missing_ok argument, which feels super confusing as it
applies only to the underlying external TOAST, if the relation has
any.  I'd be tempted to rewrite this portion of the patch with a
uint32 flags.  Even if we assign one value for now (say MISSING_TOAST,
MISSING_TOAST_OK or whatever), it would allow more flexibility on ABI
grounds if we want more like states in the future across this portion
of the stack.  Again, I strongly doubt that we will be able to
backpatch any of that.

Yeah, boolean arguments in general can be confusing. Especially
arguments like "missing_ok" where it's hard to remember which behavior
is true and which is false. An IDE that shows the name of the argument
helps, but having the caller say "MISSING_TOAST_OK" rather than "true"
is much clearer.

For backpatching, I think we could smuggle the flag in a global
variable, to avoid changing the signature of the
relation_fetch_toast_slice() callback. See attached patch 0002 on top
of your 0001 patch (which is also attached for completeness). It
doesn't fix the problem for hypothetical 3rd party tableam
implementations that implement their own relation_fetch_toast_slice()
callback. But do such extensions even exist? If yes, they could be
fixed too by also checking the global variable.

I thought about adding an extra "extended" callback next to
relation_fetch_toast_slice(), but I don't see a way to add functions
to TableAmRoutine in an ABI-compatible way. For the future, we might
want to store sizeof(TableAmRoutine) in the struct itself, so that we
could add fields to it in minor versions without breaking the API, in
case we need something like this again.

Changing `table_relation_fetch_toast_slice` signature breaks
compatibility with some extensions, for example duckdb.
I wonder if we can change only  `bool (*relation_fetch_toast_slice)(...,
bool missing_ok)` callback signature, but preserve signature of
`table_relation_fetch_toast_slice`, adding one more function
`table_relation_try_fetch_toast_slice`:

static inline void
table_relation_fetch_toast_slice(Relation toastrel, Oid valueid,
                                                      int32 attrsize,
int32 sliceoffset,
                                                      int32
slicelength, varlena *result,
                                                      bool missing_ok)
{

      (void)toastrel->rd_tableam->relation_fetch_toast_slice(toastrel,
valueid,
attrsize,
sliceoffset, slicelength,
result, false);

}

static inline bool
table_relation_try_fetch_toast_slice(Relation toastrel, Oid valueid,
                                                      int32 attrsize,
int32 sliceoffset,
                                                      int32
slicelength, varlena *result,
                                                      bool missing_ok)
{

    return toastrel->rd_tableam->relation_fetch_toast_slice(toastrel,
valueid,
attrsize,
sliceoffset, slicelength,
result, missing_ok);
 }

On Thu, Jul 09, 2026 at 08:17:06AM +0300, Konstantin Knizhnik wrote:

Changing `table_relation_fetch_toast_slice` signature breaks compatibility
with some extensions, for example duckdb.
I wonder if we can change only  `bool (*relation_fetch_toast_slice)(...,
bool missing_ok)` callback signature, but preserve signature of
`table_relation_fetch_toast_slice`, adding one more function
`table_relation_try_fetch_toast_slice`:

Thanks for the input.

Adding more functions implies to change the size of TableAmRoutine,
which may be bad. For now I'd be tempted to just focus on how to fix
the problem on HEAD in a way we are happy with. We could sort out an
optional back-branch version as a second step.
--
Michael