Hi, On Wed, Aug 5, 2026 at 9:39 PM Michael Paquier <[email protected]> wrote: > > Seems like my keyboard has slipped a bit here. Will fix after > replying to your other comments.
Thanks Michael for working on this feature. I started looking at the patches and TOAST code. Here's my take. I understand that when 4-byte chunk_id exhaustion happens, query (insert/update) latencies can increase from milliseconds to minutes and hours, or even days as OID use approaches the 4 billion limit. Although this seems to happen rarely (insert- or update-heavy workloads with more toastable columns, i.e., more chunk_id use per row, or dead tuples in the TOAST table when vacuum is busy or yet to get to the TOAST table and index), when it does happen, the mitigation for the customer seems hard: partition the table or fix vacuum. I believe that 8-byte chunk_id is the right direction to solve this problem in a simpler way, without affecting existing 4-byte chunk_id tables across upgrades and providing a simple path for customers to migrate to 8-byte chunk_id. My reading of the patches says that it follows these principles. I first tried to reproduce the query latency increase problem with less time and disk space. Here are the results. I ensured each insert steps over K ids using pg_resetwal, which equals the number of iterations spent in GetNewOidWithIndex(). There's a clear benefit as the 8-byte chunk_id avoids the retries while getting the new OID, and since a 64-bit OID is almost never exhausted, this works. Attached the test script that I used for reference. K HEAD_INSERT patched_INSERT 1,000,000 1.25 s 1.0 ms 10,000,000 12.4 s 1.0 ms 50,000,000 1 min 1.0 ms 100,000,000 2 min 1.0 ms 200,000,000 9 min 0.9 ms With this context, I started to review the patches. One thing I liked is the way the patches were split; they made review a lot easier. Here are some comments. Comments on 0001: 1/ +table_relation_fetch_toast_slice(Relation toastrel, Oid8 valueid, While changing Oid to Oid8 is fine for internal functions, is it okay to change it for the table_relation_fetch_toast_slice table AM? Is there any chance that an external table AM forgets to update? I'm not arguing against this change, just checking if there's any way out here. Of course, the compiler will generate an incompatible function parameter type warning, and since the change is going in a major release, that seems fine. One idea could be to have a TOAST version field elsewhere to detect such changes, but that seems overkill. So, having it like 0001 seems fine to me unless others have any thoughts. PS: one option is to send the 8-byte OID via a caller-allocated varlena result pointer, but this seems a bit ugly. 2/ @@ -4997,7 +4997,7 @@ ReorderBufferToastInitHash(ReorderBuffer *rb, ReorderBufferTXN *txn) Assert(txn->toast_hash == NULL); - hash_ctl.keysize = sizeof(Oid); + hash_ctl.keysize = sizeof(Oid8); Widening the per-toast-chunk hash key to an 8-byte OID still handles 4-byte chunk_ids correctly during logical decoding, since the smaller values zero-extend. The tradeoff is that with an 8-byte key the hash no longer uses the uint32_hash fast path and falls back to tag_hash, so even existing 4-byte chunk_id tables lose that optimization during logical decoding. I haven't measured the effect, but it seems worth checking. 3/ Rest of the 0001 changes look good to me, although they use an 8-byte format specifier for error reporting, which can still work for 4-byte chunk_ids. I think 0002 and 0003 are purely mechanical; they look good to me and can go in first. Comments on 0004: 1/ - WHERE o.chunk_id != pg_column_toast_chunk_id(c.value); + WHERE o.chunk_id::oid8 != pg_column_toast_chunk_id(c.value); Changing the return value of pg_column_toast_chunk_id() to 8-byte OID is fine and it can work for existing 4-byte chunk_ids. Do we need the above typecasting in tests given the chunk_id is captured from pg_column_toast_chunk_id() while creating the table? Comments on 0005: 1/ + return murmurhash64(DatumGetObjectId8(datum)); I think having catalog cache support for 8-byte OID might be useful on its own and it looks good to me except the following: can we typecast the Oid8 to murmurhash64((uint64) DatumGetObjectId8(datum))? Comments on 0008: 1/ I think adding reloption seems okay to me, so the 0008 patch looks good. Just one suggestion: having a GUC to set it once for all new tables seems a good idea as it avoids application changes, but starting with reloption is good enough. I will take a look at 0006, 0007, 0009, 0010, 0011 in the coming weeks. One more thought: existing tables won't get 8-byte chunk_ids as part of pg_upgrade. pg_dump and pg_restore could be used after the upgrade for existing customers to get their tables onto this format. At some point, providing concurrent repack-like support (or reusing the underlying machinery) to do this online would be nice to have. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
