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
