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

Hi,

On Fri, Sep 11, 2026 at 8:44 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> 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.

Upon thinking more, I think we need a test case where pg_resetwal sets
the OID8 counter beyond the OID4 limit of 4 billion and verify that
the TOAST chunk_id goes beyond that to properly cover the OID8 case.
The tests added by v17-0004-Add-battery-of-tests-related-oid8.patch,
although they create a table with OID8 TOAST, all use chunk_ids within
the OID4 limits.

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