Feedback-ID: i675e48f3:Fastmail
MIME-Version: 1.0
Date: Mon, 14 Sep 2026 10:01:15 -0400
From: "Greg Burd" <greg@burd.me>
To: "Michael Paquier" <michael@paquier.xyz>,
 "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com>
Cc: "Yugo Nagata" <nagata@sraoss.co.jp>,
 "PostgreSQL Hackers" <pgsql-hackers@lists.postgresql.org>
Message-Id: <c4fee437-ea3d-446c-84dd-5e105afa0b2c@app.fastmail.com>
In-Reply-To: <aqd_gYs35v--sVlR@paquier.xyz>
References: 
 <CALj2ACWTM=VgjDU=2BBuYAh2Ae8Kq5NspJZsmhiAvNQkXZ=MjQ@mail.gmail.com>
 <aqCvLyFwAwviycfu@paquier.xyz>
 <4272005a-ca62-46be-aab3-8678187d84cd@app.fastmail.com>
 <e971c02b-3472-4f94-9259-a0c70675206e@app.fastmail.com>
 <aqHpYL-ytLbT3jGR@paquier.xyz> <aqJhmsEEiAUKQFXN@paquier.xyz>
 <CALj2ACWEY0HHREE5Dn9Waa33coj-SwxPRkEtW4A=nCSzN8UrAQ@mail.gmail.com>
 <CALj2ACVG9cjxr-0jb8PP_Vb7vJvVGBVM_fQ2srt4CpyURpfYhQ@mail.gmail.com>
 <aqOu0TEpCM_oPksP@paquier.xyz>
 <CALj2ACVCicT3mWFD4Y-QsR+67THZ8J3FPmFtpTns2JxG3udGUQ@mail.gmail.com>
 <aqd_gYs35v--sVlR@paquier.xyz>
Subject: Re: Support for 8-byte TOAST values, round two
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/c4fee437-ea3d-446c-84dd-5e105afa0b2c%40app.fastmail.com>
Precedence: bulk


On Mon, Sep 14, 2026, at 1:00 AM, Michael Paquier wrote:
> On Fri, Sep 11, 2026 at 08:44:26AM -0700, Bharath Rupireddy wrote:
>> I reviewed v17 patches, overall they look good to me.
>> 
>> v17-0001-Refactor-some-TOAST-value-ID-code-to-use-Oid8-in.patch: It
>> v17-0002-Switch-pg_column_toast_chunk_id-return-value-fro.patch: It
>
> I have discarded the change of reorderbuffer.c that was in v17-0001,
> and added it back to what's now v18-0001.  Then applied these two
> after an extra round of self-review.
>
>> 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?
>
> If one wants to control which attribute number to look after, that
> could be extended more later on, yes.  I have removed the argument
> from the function in v18-0001.
>
>> v17-0004-Add-battery-of-tests-related-oid8.patch: It looks good to me.
>
> Okay, thanks.
>
>> 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.
>
> Nah, I don't buy that this is worth the cycles.  If one has the idea
> to create an oid8 TOAST table, inserts more than 4 billion values into
> it, dumps and restores into a table that uses a oid TOAST then the
> result is an infinite loop in the backend on restored INSERTs.
>
> Something that I still see as a hole is that we have the tables from
> the strings test in the main regression test suite and we could
> exploit that a bit more.  Hmm.  How about resetting the OID in the
> control file earlier in the pg_upgrade test and check with a SELECT
> post-upgrade that the counters generated by the main regression test
> suite are more than 2^32?
>
> Added also some tests for REPACK here in what's now 0002, based on the
> feedback of upthread, to check that the a REPACK's rewrite does not
> touch the TOAST table definition.
>
>> v17-0005-Add-support-for-TOAST-pointers-as-oid8.patch: It looks good to me.
>
> And rebased is a v18 with the three remaining pieces, plus two more
> test patches: the tests of Rui with decoding and oid8 values higher
> than 2^32, and my test to have the main regression test suite of
> pg_upgrade run with a default oid8 in the control file higher than
> 2^32, checking that assigned chunk numbers are handled correctly.
> --
> Michael
>
> Attachments:
> * v18-0001-Add-support-for-oid8-TOAST-values.patch
> * v18-0002-Add-battery-of-tests-related-oid8.patch
> * v18-0003-Add-support-for-TOAST-pointers-as-oid8.patch
> * v18-0004-Decode-an-oid8-TOAST-value-with-an-ID-past-2-32.patch
> * v18-0005-Add-check-for-2-32-oid8-chunk-values-across-the-.patch
> * signature.asc

Hello Michael, all,

I tried v18 out, as expected patches/tests/etc. all good on my macOS/arm64
laptop.

I agree with Bharath that dropping the attnum argument from
toast_valueid_scankey_init() was right, and I see it is gone in 0001.

I like what you did in 0005 with the test, moving the pg_resetwal call ahead
of the main regression suite means the whole suite now runs with the counter
already past 2^32, rather than just one case checking a large value.

  ok 1 - set an 8-byte OID counter in the old instance
  ok 7 - oid8 chunk_ids are past 2^32
  ok 25 - 8-byte OID counter is carried over by pg_upgrade

I'd say that closes the gap Bharath identified, where every oid8 test still
produced chunk_ids that fit in four bytes. Rui's decoding test in 0004 covers
the other half, and I checked separately that it fails when it should: with
ReorderBufferToastAppendChunk() put back to the truncating 4-byte read it
reports

  not ok 23 - oid8 TOAST value past 2^32 is decoded in full
  # table public.toasted_oid8: INSERT: id[integer]:1 data[text]:unchanged-toast-datum

and passes again once restored.

The REPACK additions in 0002 match what I measured by hand: with the reloption
flipped, VACUUM FULL, CLUSTER and REPACK all leave the TOAST table's chunk_id
type alone.

A quick check of my own on top of v18, counter preset to 5000000000: chunk_ids
5000000005 through 5000000054, all past 2^32, data intact, verify_heapam()
reports nothing on either the TOAST relation or the main table, and REPACK
leaves chunk_id as oid8 with the data still intact.

Nothing blocking from me on v18.

best.

-greg