From 970a30586cb141eea0dbc3f08cf4d562dc479575 Mon Sep 17 00:00:00 2001 From: Sami Imseih Date: Tue, 22 Sep 2026 17:01:29 +0000 Subject: [PATCH v1 1/1] Fix REPACK (CONCURRENTLY) for columns added without a table rewrite When applying the concurrent data changes, REPACK deformed the decoded tuples with the descriptor of the transient relation. That descriptor has no "missing" values, because make_new_heap() gives the transient relation none of the defaults and constraints of the source relation. A tuple written before an ALTER TABLE ... ADD COLUMN that did not rewrite the table has fewer attributes than the descriptor, so the missing values came out as NULL, even in a column declared NOT NULL. Such tuples will be rare, because new row versions are normally formed with the current descriptor. A BEFORE ROW UPDATE trigger returning OLD is one way to produce one. Fix by deforming the decoded tuples with the descriptor of the source relation, which does have the missing values. Tuples formed with it are still valid for the transient relation, whose attributes are a copy of the source relation ones. Commit 20d3fe9009d took the same approach for INSERT and UPDATE in the executor. Expanding the decoded tuple with heap_expand_tuple() would be the other option, but that is the workaround of commit ba9f18abd that 20d3fe9009d got rid of, so do not bring it back. Add an isolation test. --- src/backend/commands/repack.c | 24 ++++-- src/test/modules/injection_points/Makefile | 1 + .../expected/repack_missingval.out | 46 ++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/repack_missingval.spec | 85 +++++++++++++++++++ 5 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 src/test/modules/injection_points/expected/repack_missingval.out create mode 100644 src/test/modules/injection_points/specs/repack_missingval.spec diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 759be53d6b8..4e21c9f80bb 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -109,6 +109,12 @@ typedef struct ChangeContext /* The relation the changes are applied to. */ Relation cc_rel; + /* + * The relation the changes were decoded from. Its descriptor is the one + * to deform the decoded tuples with, see apply_concurrent_changes(). + */ + Relation cc_src_rel; + /* Needed to update indexes of cc_rel. */ ResultRelInfo *cc_rri; EState *cc_estate; @@ -199,6 +205,7 @@ static void process_concurrent_changes(XLogRecPtr end_of_wal, bool done); static void initialize_change_context(ChangeContext *chgcxt, Relation relation, + Relation src_relation, Oid ident_index_id); static void release_change_context(ChangeContext *chgcxt); static void rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap, @@ -2688,12 +2695,15 @@ apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt) bool have_old_tuple = false; MemoryContext oldcxt; - spilled_tuple = MakeSingleTupleTableSlot(RelationGetDescr(rel), - &TTSOpsVirtual); + /* + * Deform the decoded tuples with the descriptor of cc_src_rel, which is + * the only one that has the missing values. Tuples formed with it are + * still valid for cc_rel, whose attributes are a copy of cc_src_rel's. + */ + spilled_tuple = MakeSingleTupleTableSlot(RelationGetDescr(chgcxt->cc_src_rel), &TTSOpsVirtual); ondisk_tuple = MakeSingleTupleTableSlot(RelationGetDescr(rel), table_slot_callbacks(rel)); - old_update_tuple = MakeSingleTupleTableSlot(RelationGetDescr(rel), - &TTSOpsVirtual); + old_update_tuple = MakeSingleTupleTableSlot(RelationGetDescr(chgcxt->cc_src_rel), &TTSOpsVirtual); oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(chgcxt->cc_estate)); @@ -3156,9 +3166,11 @@ process_concurrent_changes(XLogRecPtr end_of_wal, ChangeContext *chgcxt, bool do */ static void initialize_change_context(ChangeContext *chgcxt, - Relation relation, Oid ident_index_id) + Relation relation, Relation src_relation, + Oid ident_index_id) { chgcxt->cc_rel = relation; + chgcxt->cc_src_rel = src_relation; /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); @@ -3377,7 +3389,7 @@ rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap, get_rel_name(identIdx)); /* Gather information to apply concurrent changes. */ - initialize_change_context(&chgcxt, NewHeap, ident_idx_new); + initialize_change_context(&chgcxt, NewHeap, OldHeap, ident_idx_new); /* * During testing, wait for another backend to perform concurrent data diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 9d8b4b3540c..bb70049f75d 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -19,6 +19,7 @@ ISOLATION = basic \ reindex_concurrently_deferred \ repack \ repack_decode \ + repack_missingval \ repack_temporal \ repack_temporal_multirange \ repack_toast \ diff --git a/src/test/modules/injection_points/expected/repack_missingval.out b/src/test/modules/injection_points/expected/repack_missingval.out new file mode 100644 index 00000000000..d51aa616a5c --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_missingval.out @@ -0,0 +1,46 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_wait_before_lock s2_update s2_wakeup_before_lock s1_check +injection_points_attach +----------------------- + +(1 row) + +step s1_wait_before_lock: + REPACK (CONCURRENTLY) repack_test; + +step s2_update: + UPDATE repack_test SET j = j WHERE i IN (1, 2); + +step s2_wakeup_before_lock: + SELECT injection_points_wakeup('repack-concurrently-before-lock'); + +injection_points_wakeup +----------------------- + +(1 row) + +step s1_wait_before_lock: <... completed> +step s1_check: + SELECT i, j, c, d FROM repack_test ORDER BY i; + + WITH s AS MATERIALIZED (SELECT c, d FROM repack_test) + SELECT count(*) FROM s WHERE c IS NULL OR d IS NULL; + +i|j| c|d +-+-+--+--- +1|1|42|xyz +2|2|42|xyz +3|3|42|xyz +(3 rows) + +count +----- + 0 +(1 row) + +injection_points_detach +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 80a09f34d78..7177ad32567 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -48,6 +48,7 @@ tests += { 'reindex_concurrently_deferred', 'repack', 'repack_decode', + 'repack_missingval', 'repack_temporal', 'repack_temporal_multirange', 'repack_toast', diff --git a/src/test/modules/injection_points/specs/repack_missingval.spec b/src/test/modules/injection_points/specs/repack_missingval.spec new file mode 100644 index 00000000000..efbe4d0b8b4 --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_missingval.spec @@ -0,0 +1,85 @@ +# REPACK (CONCURRENTLY); +# +# Test columns whose values are "missing" from the existing tuples, because +# ALTER TABLE ... ADD COLUMN did not have to rewrite the table. +setup +{ + CREATE EXTENSION IF NOT EXISTS injection_points; + + CREATE TABLE repack_test(i int PRIMARY KEY, j int); + INSERT INTO repack_test(i, j) VALUES (1, 1), (2, 2), (3, 3); + + -- A constant default does not rewrite the table, so the rows above keep + -- their shorter tuples and the values of "c" and "d" are only stored in + -- pg_attribute.attmissingval. Use both a pass-by-value and a + -- pass-by-reference type. + ALTER TABLE repack_test ADD COLUMN c int NOT NULL DEFAULT 42; + ALTER TABLE repack_test ADD COLUMN d text NOT NULL DEFAULT 'xyz'; + + CREATE FUNCTION repack_return_old() RETURNS trigger + LANGUAGE plpgsql AS $$ + BEGIN + RETURN OLD; + END; + $$; + + -- By returning OLD, the trigger makes the new row version reuse the + -- shorter tuple, which is then what logical decoding sees. + CREATE TRIGGER return_old BEFORE UPDATE ON repack_test + FOR EACH ROW EXECUTE FUNCTION repack_return_old(); +} + +teardown +{ + DROP TABLE repack_test; + DROP FUNCTION repack_return_old(); + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + SELECT injection_points_set_local(); + SELECT injection_points_attach('repack-concurrently-before-lock', 'wait'); +} + +# Perform the initial load and wait for s2 to change the data. +step s1_wait_before_lock +{ + REPACK (CONCURRENTLY) repack_test; +} + +# The missing values must have survived the concurrent changes. The second +# query needs the MATERIALIZED CTE because the planner would otherwise use +# attnotnull to answer it without looking at the heap. +step s1_check +{ + SELECT i, j, c, d FROM repack_test ORDER BY i; + + WITH s AS MATERIALIZED (SELECT c, d FROM repack_test) + SELECT count(*) FROM s WHERE c IS NULL OR d IS NULL; +} +teardown +{ + SELECT injection_points_detach('repack-concurrently-before-lock'); +} + +session s2 + +# The trigger returns OLD, so these rows keep the tuples they had before the +# ALTER TABLE ... ADD COLUMN above. +step s2_update +{ + UPDATE repack_test SET j = j WHERE i IN (1, 2); +} +step s2_wakeup_before_lock +{ + SELECT injection_points_wakeup('repack-concurrently-before-lock'); +} + +# Test if the missing values find their way into the repacked table. +permutation + s1_wait_before_lock + s2_update + s2_wakeup_before_lock + s1_check -- 2.50.1