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. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
