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