On Tue, Sep 15, 2026, at 3:08 AM, Michael Paquier wrote: > On Mon, Sep 14, 2026 at 12:26:54PM -0700, Bharath Rupireddy wrote: >> v18-0001-Add-support-for-oid8-TOAST-values.patch: It looks good to me. >> v18-0002-Add-battery-of-tests-related-oid8.patch: It looks good to me. >> v18-0003-Add-support-for-TOAST-pointers-as-oid8.patch: It looks good to me. >
Hello Michael, > One thing that neither you nor Greg pointed out is a disturbance I > have found in amcheck. Fair hit, and thanks for catching it. I read that hunk across several versions and never questioned the direction of the inference. Deriving the expected chunk size from the vartag is backwards in a corruption checker, since the vartag is part of what is under suspicion; the chunk_id type from the cached toast_rel is the trustworthy side, with the vartag then checked against it. I will be looking at amcheck differently from now on. > To me, applying the value set in the reloption at the moment of the dump > and not the chunk_id type feels like a natural thing to do. We still have > time to decide which behavior makes more sense for this release, but > choosing what the reloption holds at the time of the dump is a property > I'd prefer keep. I confirmed Bharath's case on f292187f375: create with oid8, load values with the counter past 2^32, then ALTER TABLE t8 RESET (toast_value_type); leaves reloptions null while chunk_id stays oid8, pg_dump then emits no WITH clause at all, and the restore comes back with an oid TOAST table. Data is intact and the IDs are simply reassigned small, so on a table this size nothing is lost. The part that gives me pause is the interaction with the case you raised upthread yourself: an oid8 table with more than four billion live values, dumped and restored into an oid TOAST table, spins in GetNewOidWithIndex(). RESET is the one step that gets a user there without ever asking for it. The dump carries no trace of the property that was dropped, so the first sign of trouble is a restore that does not finish, on exactly the tables big enough to have needed oid8. I do not think that argues against your semantics for the reloption in general, which I agree read naturally. It argues that a reset on a relation whose TOAST relation is already oid8 is the case worth treating specially. One observation on Bharath's 0001 while I am here: I do not think it actually conflicts with what you wrote. It applies the reloption when one is present, and only reads pg_attribute when there is none, so "applying the value set in the reloption at the moment of the dump" still holds under it. The two of you differ over the reloption-absent case, where there is no value set to apply, rather than over which of the two wins. Attached is a much smaller thing, offered to make the discussion concrete rather than as the answer. Report the loss at the point where it happens, and leave pg_dump alone entirely. =# ALTER TABLE t8 RESET (toast_value_type); WARNING: TOAST relation of "t8" keeps "oid8" as the type of its "chunk_id" DETAIL: The type in use is only read when the TOAST relation is created. HINT: Set "toast_value_type" to "oid8" to have a dump and restore preserve it. Only a reset naming toast_value_type reports; resetting other options on the same relation stays quiet, as does a reset on a relation whose TOAST relation uses oid, and the documented upgrade path of setting oid8 is untouched. Worth noting that the tests you committed in c68cba09dd7 already reset the option on toasttest_oid8, so the warning lands there and the expected output records it, which is the whole change outside tablecmds.c. If you would rather the dump carry the type, Bharath's 0001 is the better patch and this one should be dropped. My preference is either of them over the current behaviour. His 0002 on the migration docs seems worth having either way, and independent as you say. best. -greg
0001-Warn-when-a-reset-of-toast_value_type-drops-oid8-fro.patch
Description: Binary data
