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

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