Re: Support for 8-byte TOAST values, round two

On Thu, Sep 10, 2026 at 02:14:00PM -0700, Bharath Rupireddy wrote:
> 1/ Typo: "these can have different sizes."
> 2/ Typo: "By default these values are chosen so that four chunk rows
> will fit on a page..."?
> 6/ Missing typedefs.list entry for varatt_external_oid8.

Fixed.

> 3/ Can these be Oid instead of uint32?
>
> + uint32 va_valueid_lo; /* Low 32 bits of value ID */
> + uint32 va_valueid_hi; /* High 32 bits of value ID */

An Oid8 is not a subset of two Oids, so nope.

> 4/ Unlike ONDISK_OID which needs to be 18 to detect existing on-disk
> pointers (like the comment on top of this structure definition
> explains), having ONDISK_OID8 = 4 seems fine because there is no
> backward compatibility requirement for OID8 chunk_ids yet. Is my
> understanding correct here?

18 is just used as an historical artifact. Using 4 is fine, yes.

> 5/ This looks good, but do we also need to have one for
> varatt_external_oid? I don't think so, because it has been this way
> for many years for varatt_external_oid (previously varatt_external).

Hmm. Why not. Added one with 1497ea77edd2.

> 7/ I think we can deduplicate most of the code to reduce the if (OID8)
> else (OID) branching. I tried to do so and attached a diff on top of
> v16-0007. Please have a look.
> 8/ Also, the code in toast_save_datum() now looks a bit complicated
> and duplicated, and the comment about the race condition during
> rewrite sits only in the OID8 block, which applies to both. I tried to
> deduplicate it by moving the rewrite block to a separate function in
> the attached diff. Please have a look.

OK, that's a big chunk that reduces by close to 30% my previous bigger
chunk of code. After a close read, this changes three things:
- Introduction of toast_valueid_scankey_init(). It would be weird to
add that in the last patch while this fixes some of the bloat from
"Add support for oid8 TOAST values" for the scan keys. Find that
pretty nice.
- toast_external_data and toast_external_info_get() is what reduces a
lot of the chunk of data. Hannu had an equivalent on his other thread
for the tids where he wanted to extract all the fields individually
into an intermediate structure before passing it around, instead of
having a bunch of if/else block that depend on the vartag_external.
That's actually a super nice addition.
- toast_preserve_valueid() is also something that I've found rather
nice. The inner loop handling the OID[8] assignment becomes super
minimal, with the rewrite reasoning in the new routine.

The last two points make most sense in the last patch, because they
depend on the new vartag from what I can see.

Finally, some numbers for the last patch that introduces the new
vartag (with the biggest attribute fix included in last patch):
- v17:
16 files changed, 430 insertions(+), 212 deletions(-)
- v16:
15 files changed, 661 insertions(+), 219 deletions(-)

In short you are cutting 240 lines of code for the last changes with
your suggestions, and make the code much more readable. I'd say that
this is nice.

> 2/ Also, upon ALTER TABLE setting the reloption, since the relcache
> entry gets invalidated, we would get the updated reltoastrelid, right?
> Say I change the reloption from OID to OID8, before and after the
> chunk_id type would just be OID. Is my understanding correct?

Yes. That should be what happens.

> 3/ When RelationGetToastChunkIdType() returns InvalidOid, the caller
> assumes the OID pointer size. I think that's fine, since a relation
> without a TOAST table never externalizes anything, so the pointer size
> is only the floor for inline-compression candidates and the OID vs
> OID8 difference doesn't matter. It also matches today's behavior. Is
> that the intent?

Yes, using the OID size is for HEAD, mostly. I did not want to break
that, and an oid8-based size would be an outlier, so an oid-based size
as fallback is the natural thing to do, IMO.

Attached is a rebased v17, with the last 5 patches and your
refactorings integrated in a cleaner manner, based on my points from
above.

Thanks again for all the reviews.
--
Michael