On Sat, Aug 09, 2025 at 04:31:05PM +0900, Michael Paquier wrote:
> Without an agreement about the design choices related to the first
> patches up to 0006, I doubt there this is any need to review any of
> the follow-up patches yet because the choices of the first patches
> influence the next patches in the series. Thanks for the feedback!
So, please find attached a rebased version set that addresses all the
feedback that has been provided, hopefully:
- 0001 is the requested data type for 64b OIDs, which is called here
oid8 for the data type and Oid8 for the internal name. I'm not
completely wedded to these names, but anyway. This comes with a
package that make this data type useful for other purposes than this
patch set, basically everything where I've wanted toys for TOAST:
hash/btree operators, min/max, a couple of casts (oid -> oid8,
integers, etc). The rest of the patch set builds upon that.
- 0002 changes a couple of APIs related to TOAST to use Oid8 (in
previous iterations this was uint64). In the last version, the patch
was mixing parts related to max_chunk_size, that should be split
cleanly now.
- 0003 is a preparatory change, reducing the footprint of
TOAST_MAX_CHUNK_SIZE, because we'd want to have something that changes
depending on the vartag we're dealing with.
- 0004 renames things around vartag_external to vartag_external_oid.
Based on the previous feedback, this is more aggressive now with the
renames, renaming a few more things like MAX_CHUNK_SIZE,
TOAST_POINTER_SIZE, etc.
- 0005 is the refactoring piece, that introduces toast_external.c. I
have included the previous feedback, cleaning up the code, adding more
documentation, adding a failure fallback if the vartag given in input
is incorrect, to serve as a corruption defense. And a few more
things.
- 0006 is a follow-up cleanup of varatt.h. It would make more sense
to merge that with 0005, but I've decided to keep that separate as it
makes the review slightly cleaner.
- 0007 is related to the previous feedback, and a follow-up cleanup
that could be merged with one of the previous steps. It changes the
remaining pieces of VARATT_EXTERNAL_GET_POINTER to be related to
indirect TOAST pointers, as it's only used there.
- 0008 is a previous piece, switching pg_column_toast_chunk_id() to
use oid8 as result instead.
- 0009 adds the catcache bits for OID8OID, required for toast values
lookups and deletions, in the upcoming patches. Same as previous.
- 0010 is a new piece, based on the previous feedback. This is an
independent piece that adds an extra step in binary upgrades to be
able to pass down the attribute type of chunk_id. Without oid8
support for the values, this does not bring much of course, but it's
less code churn in the follow-up patches.
- 0011 adds a new piece, based on the previous feedback, where
the existing nextOid is enlarged to 8 bytes, keeping compatibility for
the existing 4-byte OIDs where we don't want values within the [0,
FirstNormalObjectId] range. The next patches rely on the new API to
get 8-byte values.
- 0012 is a new piece requested: reloption able to define the
attribute type of chunk_id when a TOAST relation is initially created
for a table, replacing the previous GUC approach which is now gone.
- 0013 adds support for oid8 in TOAST relations, extending the new
reloption and the binary upgrade paths previously introduced.
- 0014 adds some tests with toast OID8 case, leaving some relations
around for pg_upgrade, covering the binary upgrade path for oid8.
- 0015 is the last patch, adding a new vartag for OID8. It would make
most sense to merge that with 0013, perhaps, the split is here to ease
reviews.
I have dropped the amcheck test patch for now, which was fun but it's
not really necessary for the "basics". I have done also more tests,
playing for example with pg_resetwal, installcheck and pg_upgrade
scenarios. I am wondering if it would be worth doing a pg_resetwal in
the node doing an installcheck on the instance to be upgraded, bumping
its next OID to be much larger than 4 billion, actually..
--
Michael
