From e5d25959cf8761c7dbdd7cfbb91338e94a40e8e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= Date: Sat, 3 Oct 2026 13:04:41 +0200 Subject: [PATCH] Clear out values from dropped columns during REPACK (CONCURRENTLY) When repacking a table concurrently, some border conditions can cause a concurrent update to propagate the original old tuple to the transient table (such as a trigger that returns OLD). That's essentially just wasted space, which REPACK is there to remove. Prevent that by setting the isnull flag to true for any column cataloged as dropped in tuples being "replayed" as concurrent updates. Reported-by: Radim Marek Reviewed-by: Antonin Houska Backpatch: 19 Discussion: https://postgr.es/m/CAJgoLkK2UBzB1J9buCsSUbjf7bOqz-o_0=CeiTruU789BhBw-Q@mail.gmail.com --- src/backend/commands/repack.c | 36 +++++++--- .../expected/repack_dropped.out | 47 ++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/repack_dropped.spec | 71 +++++++++++++++++++ 4 files changed, 144 insertions(+), 11 deletions(-) create mode 100644 src/test/modules/injection_points/expected/repack_dropped.out create mode 100644 src/test/modules/injection_points/specs/repack_dropped.spec diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 596c1abaf78..899005609c2 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -192,8 +192,8 @@ static void apply_concurrent_update(Relation rel, TupleTableSlot *spilled_tuple, static void apply_concurrent_delete(Relation rel, TupleTableSlot *slot); static void restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot); -static void adjust_toast_pointers(Relation relation, TupleTableSlot *dest, - TupleTableSlot *src); +static void prepare_concurrent_update(TupleTableSlot *dest, + TupleTableSlot *src); static bool find_target_tuple(Relation rel, ChangeContext *chgcxt, TupleTableSlot *locator, TupleTableSlot *retrieved); @@ -2817,13 +2817,11 @@ apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt) elog(ERROR, "could not find target tuple"); /* - * If 'tup' contains TOAST pointers, they point to the old - * relation's toast. Copy the corresponding TOAST pointers for the - * new relation from the existing tuple. (The fact that we - * received a TOAST pointer here implies that the attribute hasn't - * changed.) + * 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. */ - adjust_toast_pointers(rel, spilled_tuple, ondisk_tuple); + prepare_concurrent_update(spilled_tuple, ondisk_tuple); apply_concurrent_update(rel, spilled_tuple, ondisk_tuple, chgcxt); @@ -3015,11 +3013,20 @@ restore_tuple(BufFile *file, Relation relation, TupleTableSlot *slot) } /* - * Adjust 'dest' replacing any EXTERNAL_ONDISK toast pointers with the - * corresponding ones from 'src'. + * Adjust the tuple in 'dest' so that it can be used as the NEW tuple in an + * update that we're about to replay. + * + * We perform the following critical change: + * - 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 -adjust_toast_pointers(Relation relation, TupleTableSlot *dest, TupleTableSlot *src) +prepare_concurrent_update(TupleTableSlot *dest, TupleTableSlot *src) { TupleDesc desc = dest->tts_tupleDescriptor; @@ -3029,7 +3036,14 @@ adjust_toast_pointers(Relation relation, TupleTableSlot *dest, TupleTableSlot *s 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)) diff --git a/src/test/modules/injection_points/expected/repack_dropped.out b/src/test/modules/injection_points/expected/repack_dropped.out new file mode 100644 index 00000000000..8aaab73aebb --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_dropped.out @@ -0,0 +1,47 @@ +Parsed test spec with 3 sessions + +starting permutation: s1_size s2_repack s3_updates s1_unlock s2_noop s1_size +injection_points_attach +----------------------- + +(1 row) + +step s1_size: + SELECT id, pg_column_size(repack_dropped) < current_setting('block_size')::int + AS fits_in_one_block + FROM repack_dropped ORDER BY id; + +id|fits_in_one_block +--+----------------- + 1|f + 2|f +(2 rows) + +step s2_repack: + REPACK (CONCURRENTLY) repack_dropped; + +step s3_updates: + UPDATE repack_dropped SET a = a || a; + +step s1_unlock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step s2_repack: <... completed> +step s2_noop: + +step s1_size: + SELECT id, pg_column_size(repack_dropped) < current_setting('block_size')::int + AS fits_in_one_block + FROM repack_dropped ORDER BY id; + +id|fits_in_one_block +--+----------------- + 1|t + 2|t +(2 rows) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index db6b93a8115..b74ea0f0ca1 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -49,6 +49,7 @@ tests += { 'repack', 'repack_commit_race', 'repack_decode', + 'repack_dropped', 'repack_missingval', 'repack_temporal', 'repack_temporal_multirange', diff --git a/src/test/modules/injection_points/specs/repack_dropped.spec b/src/test/modules/injection_points/specs/repack_dropped.spec new file mode 100644 index 00000000000..099cf744202 --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_dropped.spec @@ -0,0 +1,71 @@ +# Test REPACK (CONCURRENTLY) removing values from dropped columns. +# +# The tuples are large enough to use two disk blocks; after dropping column b, +# they'll comfortably fit in a single page. The OLD tuple 1 is propagated +# through the concurrent update because of the trigger. +setup { + CREATE EXTENSION injection_points; + + CREATE TABLE repack_dropped (id int PRIMARY KEY, a text, b text); + ALTER TABLE repack_dropped ALTER COLUMN b SET STORAGE EXTERNAL; + INSERT INTO repack_dropped (id, a, b) VALUES (1, 'one', + repeat(encode(sha256('1'), 'hex'), current_setting('block_size')::int / 32)); + 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 + 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(); + ALTER TABLE repack_dropped DROP COLUMN b; +} + +teardown { + DROP TABLE repack_dropped; + DROP FUNCTION repack_dropped_f; +} + +session s1 + +step s1_size +{ + SELECT id, pg_column_size(repack_dropped) < current_setting('block_size')::int + AS fits_in_one_block + FROM repack_dropped ORDER BY id; +} + +step s1_unlock +{ + SELECT injection_points_wakeup('repack-concurrently-before-lock'); +} + +session s2 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('repack-concurrently-before-lock', 'wait'); +} + +step s2_repack +{ + REPACK (CONCURRENTLY) repack_dropped; +} + +step s2_noop +{ +} + +session s3 +step s3_updates +{ + UPDATE repack_dropped SET a = a || a; +} + +permutation + s1_size + s2_repack + s3_updates + s1_unlock + s2_noop + s1_size