On Thu, Aug 27, 2026 at 11:23 AM Zhijie Hou (Fujitsu) <[email protected]> wrote: > Thanks for the comments. They look good to me and I have addressed them > in V4 patch.
Thanks for working on this! I reviewed v4 and have two comments for now. 1. The extra logging does not actually depend on the key changing. ``` + old_key_tuple = BuildOldKeyTuple(relation, &oldtup, newtup, + bms_overlap(modified_attrs, id_attrs) || + id_has_external, + log_unchanged_external && + HeapTupleHasExternal(&oldtup) && + HeapTupleHasExternal(newtup), + &old_key_copied); ``` The new columns are only kept when BuildOldKeyTuple() gets past its `if (!key_required) return NULL`, and the key_required passed above is `bms_overlap(modified_attrs, id_attrs) || id_has_external`, so id_has_external alone gets it there. HeapDetermineColumnsInfo() sets id_has_external when a replica identity column of the old tuple is stored externally, with no key change at all. So on a table whose replica identity covers a toasted column and which has another toasted column outside it, an UPDATE that touches neither cannot be transformed, yet still detoasts and flattens that column into the WAL record. 2. The test never checks that val is actually stored out-of-line. If that ever changed, the test would keep passing without exercising the fix, since an inline value replicates fine anyway. Asserting pg_column_toast_chunk_id(val) IS NOT NULL on the publisher before the UPDATE would pin that down. -- Shinya Kato NTT OSS Center
