Hi, Michael !
While looking at v16-0007, one thing looked suspicious to me.
VARATT_IS_EXTERNAL_ONDISK() used to effectively mean that the datum
could be interpreted as the existing OID-based external pointer. With
OID8 it becomes a family predicate, accepting both OID and OID8
pointers.
This looked like a potentially silent compatibility problem for
extensions: old code may still compile, but its assumption about the
pointer layout is no longer true.
I did a quick check of out-of-tree users and found at least two
examples. PostGIS has code like this in ToastCacheGetGeometry():
if (!VARATT_IS_EXTERNAL_ONDISK(attr))
...
struct varatt_external ve;
VARATT_EXTERNAL_GET_POINTER(ve, attr);
Oid valueid = ve.va_valueid;
Oid toastrelid = ve.va_toastrelid;
MobilityDB carries the same pattern. So an OID8 datum can now pass
VARATT_IS_EXTERNAL_ONDISK() and existing code can then interpret it
using the old OID pointer layout.
I noticed this because we have been experimenting with alternative
physical representations of large varlena values and tracing which
parts of the value lifecycle actually depend on a particular TOAST
representation. OID8 turned out to be a useful real-world test of that
boundary.
Perhaps we need to distinguish explicitly between "any on-disk
external pointer" and "the old OID on-disk pointer", rather than
changing the meaning of the existing predicate?
-Oleg
On Wed, Sep 9, 2026 at 6:43 AM Michael Paquier <[email protected]> wrote:
>
> 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
--
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
