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

On Thu, Sep 10, 2026 at 10:06:07AM -0700, Bharath Rupireddy wrote:
> On Thu, Sep 10, 2026 at 12:52 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> Attached is a rebased v16 following cb298616463d. The patch set has
>> been slightly re-organized, with the pieces that seem worth
>> introducing first on top of the rest:
>> - 0001 for binary upgrades.
>
> 0001 looks good to me. I reviewed v15 previously and there is no change in v16.

Applied that after a few tweaks, mostly some contents in pg_dump I
forgot to update.

>> - 0002 for the reloption toast_value_type
>
> 0002 looks good to me. The note in the docs about rewrites and pg_dump
> and the tests are good additions. My previous comments on
> parenthesizing the default arg and the typo in the docs are fixed.

Still missed an update of typedefs.list (not mandatory per-se, but I
tend to prefer clean indentation from the start).

>> - 0005 for adding toast_value_type=oid8, with all the TOAST changes
>> not related to the new vartag.
>
> 0005 looks good to me with the following comment. With this, one can
> create a TOAST table with OID8 chunk_id but will only get OID4
> behavior.

And that's exactly the point. In understand that I may be picky
regarding these splits, but I prefer this order rather than the
opposite of introducing the vartag and then the interfaces due to one
reason: untestable and not reachable dead code. Having 4-byte OID
pointers as a temporary measure for oid8 tables is not that bad in
comparison..

> Calling that out in the commit message seems fine to me, but
> let us try to get this patch and 0007 (adding vartag_external) closer
> or together. Also, the mention about the price one has to pay for
> using OID8 (four extra bytes per out-of-line pointer) looks like it
> addresses Greg's point on the max number of TOAST-able columns.

Will improve.

> 1/ An error would be better here when an unexpected TOAST typeid is
> detected for some reason.
> + else
> + Assert(false);

Yeah, I don't like much this pattern based on typid or even vartag
lookups. Will improve that a bit, probably without these asserts.

>> - 0006 is the battery of tests for all the cases found during
>> development, grouped together: core things, amcheck, test_decoding..
>
> 0006 looks good to me. Tests cover most of the cases. Is it good to
> cover the pg_dump and pg_restore case (OID4 to OID8 after changing the
> reloption) as well, perhaps as a follow-up patch?

Yes, I guess I should. One thing that slightly annoys me is that we
don't have zero dump/restore tests for TOAST tables in binary upgrade
mode, and no checks related to their relfilenode and with their SQL
functions produces.. That's not the problem of this patch set, but
let's close the gap anyway as an initial, separate, improvement.

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

Thanks again for the reviews. I still need to follow with your second
mail, which will include a rebase and all the comments of this message
addressed.
--
Michael