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

On Tue, Sep 08, 2026 at 04:55:16PM -0700, Bharath Rupireddy wrote:
> I don't think pg_control_checkpoint() and pg_controldata emitting an
> OID extracted from the OID8 value is the right idea. I understand that
> tools and external modules do need to adapt to the wider value, but
> that is fine for a new major version, and the control file version
> bump gives them the signal to do so. Also, the extra 4 bytes from OID8
> are absorbed by existing padding, so the control file stays well under
> the safe size limit of 512 bytes anyway.

I don't mean to extend the on-disk file with an extra 4-bytes for the
Oid. I mean to *show* an extra field based on a casted value. These
are two different things.

> v15-0001-Enlarge-OID-generation-to-8-bytes.patch: this looks good to
> me, with one comment. Although the wraparound with an 8-byte OID is
> hundreds of years away, and 0001 assumes it never happens, do we need
> to handle the case where someone sets the OID counter closer to the
> 8-byte OID limit using pg_resetwal and still hits the wraparound? We
> can either reject this in pg_resetwal, handle the wraparound case in
> the caller, document it, or mention it in the commit message or
> somewhere near pg_resetwal. This is not a blocker for getting 0001 in,
> just to be clear. I think this needs to be fixed as part of
> v15-0006-Add-support-for-oid8-TOAST-values.patch, which does not check
> for an existing TOAST OID.

A few things worth noting: we have in xlog_redo() and varsup.h a
couple of comments that mention wraparound without mentioning the oid8
bits that can bypass it. These need to be slightly adjusted..

I can fancy you with one more case, because there is no entry recheck
in the TOAST insert path when using an 8-byte OID:
- Insert some TOAST entries with toast_value_type=oid8.
- Reset to a past value with pg_resetwal.
- Reinsert, failing hard on INSERT due to a duplicated key.

My best answer is don't set the counter to the past. Somebody using
pg_resetwal is looking for trouble already. It's true that not being
able to handle the OID8 value conflicts like the normal TOAST path
could be seen as a defect of the OID8 path, but adding an index check
(while doable of course), is just wasting resources with 8-bytes,
because in practice that's never hit. So "don't set it to the past"
seems like an answer good enough? That sounds to me like a
documentation change.

> v15-0003-Switch-pg_column_toast_chunk_id-return-value-fro.patch: this
> looks good to me, with one comment. The chunk_id cast in
> cluster-toast-value-reuse.spec seems redundant, because that column
> comes from the output of pg_column_toast_chunk_id(). In
> misc_functions.sql it is needed. This makes me think about whether we
> need to support OID to OID8 comparison in more cases: existing queries
> and monitoring tools that do pg_column_toast_chunk_id(x) = some_oid,
> or that store the result in an OID column, will now get a
> type-mismatch error and have to add an explicit cast. Could we ease
> that by making the OID to OID8 cast implicit, so that an OID compared
> against an OID8 is promoted automatically? This is not a blocker for
> getting 0003 in, just to be clear.

My reply here would be to cast the some_oid to oid8, or invent a
secondary oid8 specific function, leaving pg_column_toast_chunk_id()
alone and failing if trying to use it with an oid8 TOAST table?

I suspect that there are not that many users of
pg_column_toast_chunk_id() anyway. May be wrong of course, but a cast
to oid8 is fine by me. I'm just not sure that it is worth the churn
of having a second function with a different return type.

> v15-0004-Add-support-for-TOAST-chunk_id-type-in-binary-up.patch: this
> looks good to me, with a nit. Use
> format_type_be(binary_upgrade_next_toast_chunk_id_typoid) instead of
> %u for better readability in the "cannot support toast chunk_id type"
> error message.

Not sure it matter. That should not be reachable in practice.

> v15-0005-Add-relation-option-toast_value_type.patch: some comments.
> 1/ Parenthesize defaulttarg.
> 2/ Nit: double space after "creating".

Noted these down for later.

> 3/ I understand that v15-0008 adds more tests, but can we have simple
> tests in this patch as well, like creating a table with
> toast_value_type='oid' and with an unknown type to verify the error?
> This would make this patch independent.

Yeah, I guess that you're right to complain here. Most of the
reloption tests could be moved to the patch that introduces the
reloption, for clarity.

> 5/ Following on from (4). Say I create a table with oid, insert data,
> change the reloption to oid8, and then run a rewrite (VACUUM FULL,
> CLUSTER or REPACK). The final table still uses oid, because the
> rewrite preserves the existing TOAST table by swapping contents rather
> than recreating it. But the transient table that make_new_heap()
> builds does get created with the new type, because it goes through
> create_toast_table(), which reads the reloption without checking
> whether this is a rewrite. With oid this makes no difference, but once
> oid8 is added the transient TOAST table would be oid8 while the
> existing TOAST table still says oid. Is that expected?

I am not completely sure to follow here? Wouldn't that be the case of
create_toast_table() defining a OIDOldToast. In this case, we inherit
the type ID from the old table when creating what would be the
transient table. See the comment named as "break the world" in
create_toast_table() added in the patch. Or perhaps I'm missing your
point entirely.

> 1/ Measured the additional storage space and impact on query
> performance for the same TOAST limits under 4 billion (OID limit).
>
> At 4 GB TOAST (500k rows × 8 KB), oid8 costs +40% in TOAST index size
> and +15% in heap size, with no measurable change to the TOAST table
> size or full-detoast scan time. Queries [1]. The heap increase comes
> from the on-disk TOAST pointer growing 4 bytes and the tuple
> alignment. The TOAST index grows for the same reason on its chunk_id
> key. The TOAST table size stays about the same because the extra
> chunk_id bytes are small next to the roughly 2 KB of chunk_data and do
> not change how many chunks fit per page.

Yeah, the cost is part of the game here..

> 2/ Used oid8 for all tables by default and ran regression tests to see
> if there are any issues.

Did this one as well at some point. I'm pretty sure I've caught some
of the rewrite bugs with that, but I don't recall entirely..

> [3]
> doesn't match '(?^:(?^ms:heap table "postgres\.public\.test", block 0,
> offset 5:\s+)data begins at offset 152 beyond the tuple length 58)'
> # Looks like you failed 1 test of 32.
> t/004_verify_heapam.pl ... Dubious, test returned 1 (wstat 256, 0x100)
> Failed 1/32 subtests

This needs manipulation of on-disk data. I have done that at some
point but I did not see that as worth the extra cost. Having tests
for the valid cases sounded more than enough, because we care about
making sure that the vartags are passed around right. Whether they
actual report a corruption is less interesting.

Before reworking on a new rebase of the whole patch set, attached is
an adjusted 0001 for the 8-byte counter, with adjustments for the docs
of pg_resetwal and fixes for a few comments that worry about OID
wraparound, that I have noticed after more review.

How does this part look?
--
Michael