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

On Thu, 9 Jul 2026 at 04:47, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

I've gone through the patch, see below. I've attached some of the
suggested changes in 0002, with your v4-0001 as v5-0001. Some are more
involved.

> +++ b/src/backend/access/heap/heaptoast.c
> heap_toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
> - uint32 options)
> + uint32 options, uint32 flags)

Is there a good reason why the options argument couldn't be reused
with one or a few more new flag bits? The TABLE_INSERT options itself
defines only 3 bits right now, and HEAP_INSERT adds only one more,
leaving more than enough space for any other shenanigans we may have
to pull with non-MVCC-backed TOAST operations.

> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -857,6 +865,45 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
> {
> + /*
> + * For RECENTLY_DEAD tuples, verify TOAST data is still available
> + * before putting the tuple in the sort. If a concurrent VACUUM
> + * already reclaimed it, skip the tuple.
> + */
> + if (recently_dead)
[...]

This code only skips tuples whose toasted data is not deleted ahead of
putting the tuple referencing the toasted data into the sort. It
doesn't save the detoasted values, and so doesn't really protect
against concurrent removal of the tuple's toasted data _after_ putting
the tuple into the sort, which could cause issues if the same tuple
needs to detoast now-deleted toast data as part of (e.g.) the
tuplesort.

> + *tups_vacuumed += 1;
> + *num_tuples -= 1;
> + if (recently_dead)
> + *tups_recently_dead -= 1;

I think this `if (recently_dead)` should be an Assert(recently_dead).
Of all possible tuple visibility states only RECENTLY_DEAD tuples are
allowed to get their TOAST data reclaimed while we're processing the
scan. Adjusted accordingly in 0002

> + /*
> + * Extract the set of attributes an index may need to detoast, including
> + * expressions and predicates.
> + */

I think this code will need to be adjusted to handle whole-row expressions.
Right now, it fails when an index/order by of `(tablename.*)` is
defined; this gives a Var with attno=0, which can't be detoasted as
such, but should cause every attribute to be detoasted.
Example: Use this in the 055 test instead of the current index, and
you'll probably hit a detoasting error:
'CREATE INDEX rewrite_test_data_idx ON rewrite_test ((rewrite_test
IS NOT NULL))');

Fix in 0002.

> @@ -1607,6 +1686,44 @@ heapam_index_build_range_scan(Relation heapRelation,
> continue;
> }
>
> + /* For RECENTLY_DEAD tuples, pre-detoast external TOAST values. */

This path can leak memory if multiple attributes need to be detoasted
in this path. We need to keep track of which attributes we need to
manually pfree(), so that we don't leak memory in this path.

Additionally, this pre-detoasting section needs to happen before we
execute the predicate, because right now we only detect missing toast
and reject the tuples after the predicate was executed; and this
predicate can have detoasting with ERRORs if the attributes weren't
yet pre-detoasted.
These both are fixed in 0002.

I've also included a fix for issue 1 of Zhijie's reply of July 24 in patch 0002.

Kind regards,

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