Hi Alvaro, A replayed INSERT can carry a dropped column's value too, and that path still stores it.
A BEFORE INSERT trigger that returns a copy of an existing row does it: r := (SELECT t FROM demo t WHERE id = 1); r.id := NEW.id; RETURN r; So does an UPDATE that moves the row to another partition while the trigger returns OLD. COPY, MERGE and INSERT ON CONFLICT go the same way. It can also make REPACK fail. If the columns that are left need no TOAST table, the new heap has none, and on master I get: ERROR: row is too big: size 16424, maximum size 8160 0001 clears dropped columns in restore_tuple(), so every kind of change is covered, and the block added to prepare_concurrent_update() is no longer needed. 0002 adds a concurrent INSERT to repack_dropped. I kept it apart in case you want only the fix. Thanks, Shihao
From dabe9f8196eeacf708d1dcf227efd96bcf9cbce0 Mon Sep 17 00:00:00 2001 From: shihao zhong <[email protected]> Date: Mon, 5 Oct 2026 00:18:57 -0400 Subject: [PATCH v1 2/2] Test dropped-column values in inserts replayed by REPACK Add a concurrent INSERT to repack_dropped. A BEFORE INSERT trigger makes the new row a copy of an existing one, so it carries the value of the dropped column. Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com --- .../injection_points/expected/repack_dropped.out | 8 ++++++-- .../injection_points/specs/repack_dropped.spec | 16 +++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/test/modules/injection_points/expected/repack_dropped.out b/src/test/modules/injection_points/expected/repack_dropped.out index 8aaab73aebb..4186ce4981c 100644 --- a/src/test/modules/injection_points/expected/repack_dropped.out +++ b/src/test/modules/injection_points/expected/repack_dropped.out @@ -1,6 +1,6 @@ Parsed test spec with 3 sessions -starting permutation: s1_size s2_repack s3_updates s1_unlock s2_noop s1_size +starting permutation: s1_size s2_repack s3_updates s3_insert s1_unlock s2_noop s1_size injection_points_attach ----------------------- @@ -23,6 +23,9 @@ step s2_repack: step s3_updates: UPDATE repack_dropped SET a = a || a; +step s3_insert: + INSERT INTO repack_dropped (id, a) VALUES (3, 'three'); + step s1_unlock: SELECT injection_points_wakeup('repack-concurrently-before-lock'); @@ -43,5 +46,6 @@ id|fits_in_one_block --+----------------- 1|t 2|t -(2 rows) + 3|t +(3 rows) diff --git a/src/test/modules/injection_points/specs/repack_dropped.spec b/src/test/modules/injection_points/specs/repack_dropped.spec index 099cf744202..57700968032 100644 --- a/src/test/modules/injection_points/specs/repack_dropped.spec +++ b/src/test/modules/injection_points/specs/repack_dropped.spec @@ -13,11 +13,18 @@ setup { INSERT INTO repack_dropped (id, a, b) VALUES (2, 'two', repeat(encode(sha256('2'), 'hex'), current_setting('block_size')::int / 32)); CREATE FUNCTION repack_dropped_f() RETURNS trigger LANGUAGE plpgsql AS - $$ BEGIN + $$ DECLARE r repack_dropped; + BEGIN + IF TG_OP = 'INSERT' THEN + r := (SELECT t FROM repack_dropped t WHERE id = 1); + r.id := NEW.id; + RETURN r; + END IF; IF NEW.id = 1 THEN return OLD; END IF; RETURN NEW; END $$; CREATE TRIGGER repack_dropped_t BEFORE UPDATE ON repack_dropped FOR EACH ROW EXECUTE FUNCTION repack_dropped_f(); + CREATE TRIGGER repack_dropped_ins_t BEFORE INSERT ON repack_dropped FOR EACH ROW EXECUTE FUNCTION repack_dropped_f(); ALTER TABLE repack_dropped DROP COLUMN b; } @@ -62,10 +69,17 @@ step s3_updates UPDATE repack_dropped SET a = a || a; } +# The trigger makes tuple 3 a copy of tuple 1, dropped column included. +step s3_insert +{ + INSERT INTO repack_dropped (id, a) VALUES (3, 'three'); +} + permutation s1_size s2_repack s3_updates + s3_insert s1_unlock s2_noop s1_size -- 2.37.1 (Apple Git-137.1)
From 09555e6c8b72da4a6f133f683286895c611cc4e5 Mon Sep 17 00:00:00 2001 From: shihao zhong <[email protected]> Date: Mon, 5 Oct 2026 00:18:57 -0400 Subject: [PATCH v1 1/2] Clear out dropped-column values from inserts replayed by REPACK e5d25959cf8 cleared values of dropped columns only when replaying an UPDATE. A concurrent INSERT can carry one too, for example when a trigger returns a copy of an existing row. REPACK (CONCURRENTLY) then kept it, or failed with "row is too big" if the new heap has no TOAST table. Clear them in restore_tuple() instead. Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com --- src/backend/commands/repack.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 899005609c2..7870ac6607b 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -2819,7 +2819,7 @@ apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt) /* * Adjust spilled_tuple so that it can be used as the new tuple in * the update that we're about to replay. This fixes TOAST - * pointers as well as remove useless values from dropped columns. + * pointers. */ prepare_concurrent_update(spilled_tuple, ondisk_tuple); @@ -2946,6 +2946,7 @@ apply_concurrent_delete(Relation rel, TupleTableSlot *slot) static void restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot) { + TupleDesc desc = slot->tts_tupleDescriptor; uint32 t_len; HeapTuple tup; int natt_ext; @@ -2965,6 +2966,17 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot) */ ExecForceStoreHeapTuple(tup, slot, false); + /* + * Dropped columns can still have values in the tuple. Mark them as null: + * they'd waste space, and the new heap might have no TOAST table to store + * them in. + */ + for (int i = 0; i < desc->natts; i++) + { + if (TupleDescCompactAttr(desc, i)->attisdropped) + slot->tts_isnull[i] = true; + } + /* * Next, read any attributes we stored separately into the tts_values * array elements expecting them, if any. This matches @@ -2973,8 +2985,6 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot) BufFileReadExact(file, &natt_ext, sizeof(natt_ext)); if (natt_ext > 0) { - TupleDesc desc = slot->tts_tupleDescriptor; - for (int i = 0; i < desc->natts; i++) { CompactAttribute *attr = TupleDescCompactAttr(desc, i); @@ -3020,10 +3030,6 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot) * - Any EXTERNAL_ONDISK toast pointers so that it points to the corresponding * toast value in 'src' (the transient table) instead. The TOAST storage for * 'dest' is going to be dropped, so these values cannot be used any longer. - * - * We also apply the following optimization: - * - If any columns are dropped but the slot still contains values, mark them - * as null to avoid uselessly wasting space in the new relation. */ static void prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src) @@ -3036,14 +3042,7 @@ prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src) varlena *varlena_dst; if (attr->attisdropped) - { - if (!slot_attisnull(dest, i + 1)) - { - slot_getsomeattrs(dest, i + 1); - dest->tts_isnull[i] = true; - } continue; - } if (attr->attlen != -1) continue; if (slot_attisnull(dest, i + 1)) -- 2.37.1 (Apple Git-137.1)
