Hi, On Fri, Sep 11, 2026 at 8:44 AM Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Fri, Sep 11, 2026 at 12:33 AM Michael Paquier <[email protected]> wrote: > > > > > 7/ I think we can deduplicate most of the code to reduce the if (OID8) > > > else (OID) branching. I tried to do so and attached a diff on top of > > > v16-0007. Please have a look. > > > 8/ Also, the code in toast_save_datum() now looks a bit complicated > > > and duplicated, and the comment about the race condition during > > > rewrite sits only in the OID8 block, which applies to both. I tried to > > > deduplicate it by moving the rewrite block to a separate function in > > > the attached diff. Please have a look. > > > > OK, that's a big chunk that reduces by close to 30% my previous bigger > > chunk of code. > > > > Finally, some numbers for the last patch that introduces the new > > vartag (with the biggest attribute fix included in last patch): > > - v17: > > 16 files changed, 430 insertions(+), 212 deletions(-) > > - v16: > > 15 files changed, 661 insertions(+), 219 deletions(-) > > > > In short you are cutting 240 lines of code for the last changes with > > your suggestions, and make the code much more readable. I'd say that > > this is nice. > > Nice! > > > Attached is a rebased v17, with the last 5 patches and your > > refactorings integrated in a cleaner manner, based on my points from > > above. > > I reviewed v17 patches, overall they look good to me. > > v17-0001-Refactor-some-TOAST-value-ID-code-to-use-Oid8-in.patch: It > looks good to me. No difference from that of v16 or v15 (the versions > I previously reviewed). > > v17-0002-Switch-pg_column_toast_chunk_id-return-value-fro.patch: It > looks good to me. No difference from that of v16 or v15 (the versions > I previously reviewed). > > 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? > > v17-0004-Add-battery-of-tests-related-oid8.patch: It looks good to me. > No difference from that of v16 or v15 (the versions I previously > reviewed). 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. > > v17-0005-Add-support-for-TOAST-pointers-as-oid8.patch: It looks good to me.
Upon thinking more, I think we need a test case where pg_resetwal sets the OID8 counter beyond the OID4 limit of 4 billion and verify that the TOAST chunk_id goes beyond that to properly cover the OID8 case. The tests added by v17-0004-Add-battery-of-tests-related-oid8.patch, although they create a table with OID8 TOAST, all use chunk_ids within the OID4 limits. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
