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
