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

Hi,

On Fri, Sep 11, 2026 at 12:33 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> > 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.
>
> 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.

Nice!

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

I reviewed v17 patches, overall they look good to me.

v17-0001-Refactor-some-TOAST-value-ID-code-to-use-Oid8-in.patch: It
looks good to me. No difference from that of v16 or v15 (the versions
I previously reviewed).

v17-0002-Switch-pg_column_toast_chunk_id-return-value-fro.patch: It
looks good to me. No difference from that of v16 or v15 (the versions
I previously reviewed).

v17-0003-Add-support-for-oid8-TOAST-values.patch: It looks good to me
with one nit. Since chunk_id is always at attnum = 1 and all the
toast_valueid_scankey_init() callers pass it as 1, and this init
function is just to fetch the chunk_id, can we just hard-code it
inside and remove the attnum function parameter?

v17-0004-Add-battery-of-tests-related-oid8.patch: It looks good to me.
No difference from that of v16 or v15 (the versions I previously
reviewed). pg_dump/pg_restore for demoing OID4 to OID8 migration for
existing tables and TOAST table tests during pg_upgrade could be
follow-up patches. This makes me think, if someone does an OID8 to
OID4 migration and the chunk_ids are beyond the 4-billion limit, the
restore should fail rather than silently wrapping the chunk_ids and
causing TOAST index insert failures. My point is, we need to test this
case as well.

v17-0005-Add-support-for-TOAST-pointers-as-oid8.patch: It looks good to me.

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