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

Hi,

On Thu, Sep 10, 2026 at 10:06 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> > - 0007 adds the new vartag_external.
> >
> > - 0008 is the tweak for toast_tuple_find_biggest_attribute(), which
> > should perhaps be merged with 0007.
>
> I will soon get to reviewing these two patches.

Here are some comments on 0007:

1/ Typo: "these can have different sizes."

+ * Note that TOAST pointers could have different vartags, for oid
+ * or oid8, and these can have a different sizes.

2/ Typo: "By default these values are chosen so that four chunk rows
will fit on a page..."?

Out-of-line values are divided (after compression if used) into chunks of at
-most <symbol>TOAST_OID_MAX_CHUNK_SIZE</symbol> bytes (by default this
value is chosen
+most <symbol>TOAST_OID_MAX_CHUNK_SIZE</symbol> or
+<symbol>TOAST_OID8_MAX_CHUNK_SIZE</symbol> bytes depending on the
+<structfield>chunk_id</structfield> type (by default this value is chosen
so that four chunk rows will fit on a page, making it about 2000 bytes).

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

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?

/*
* Type tag for the various sorts of "TOAST pointer" datums. The peculiar
* value for VARTAG_ONDISK_OID comes from a requirement for on-disk
* compatibility with a previous notion that the tag field was the pointer
* datum's length.
*/

+ VARTAG_ONDISK_OID8 = 4,
VARTAG_ONDISK_OID = 18
} vartag_external;

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

* This struct must not contain any padding, because we sometimes compare
* these pointers using memcmp.

+StaticAssertDecl((sizeof(int32) + 3 * sizeof(uint32) + sizeof(Oid)) ==
+ sizeof(varatt_external_oid8),
+ "varatt_external_oid8 should have no padding");

6/ Missing typedefs.list entry for varatt_external_oid8.

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.

Here are some comments on 0008:

1/ Caching the chunk_id type in the relcache entry seems fine, as it
avoids the syscache lookup costs in a relatively hot code path, for
every insert and update to a TOAST table.

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?

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?

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com