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

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);
 }

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