Hi, On Thu, Sep 10, 2026 at 10:06 AM Bharath Rupireddy <[email protected]> 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
nocfbot-oid8-dedup.diff
Description: Binary data
