On 2026-Sep-22, Sami Imseih wrote: > REPACK (CONCURRENTLY) is not correctly accounting for missing values when > applying decoded changes to the transient relation (NewHeap). This is > because it is using the NewHeap descriptor, which intentionally has no > missing values when it is formed in make_new_heap().
Hah, interesting, thanks. I wonder why in your fix we keep a pointer to the whole relation instead of just to its tupledesc. What about the attached v4? I considered using CreateTupleDescCopyConstr, but it seems pointless: the Relation pointer cannot go away while repack is running anyway. I ran your test with only CreateTupleDescCopy() to see how would your new test would fail (because such a descriptor wouldn't have the missing attrs), but it failed differently, because attnotnull is not set. Anyway, this was just a perhaps pointless experiment. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Los cuentos de hadas no dan al niño su primera idea sobre los monstruos. Lo que le dan es su primera idea de la posible derrota del monstruo." (G. K. Chesterton)
>From 3775be2f017889e2766b8b5b61ca240288f7da31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> Date: Thu, 24 Sep 2026 14:10:14 +0200 Subject: [PATCH v4] 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. If such a value belongs to the replica identity, the key used to find the target tuple was NULL too, so the tuple could not be found. The scan key does not remember that a value is NULL, so a pass-by-reference key would even crash the comparison function. 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. Author: Sami Imseih <[email protected]> Reported-by: Shihao Zhong <[email protected]> Reviewed-by: Kirill Reshke <[email protected]> Backpatch-through: 19 Discussion: https://postgr.es/m/CAN12+Y+NJwr5EqKVrHpPD=5+b_nraozu9fhrvdo3h46x2wr...@mail.gmail.com --- src/backend/commands/repack.c | 23 +++-- src/test/modules/injection_points/Makefile | 1 + .../expected/repack_missingval.out | 38 +++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/repack_missingval.spec | 83 +++++++++++++++++++ 5 files changed, 140 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..8a4dc63f37a 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -109,6 +109,9 @@ typedef struct ChangeContext /* The relation the changes are applied to. */ Relation cc_rel; + /* The tuple descriptor to deform decoded tuples with */ + TupleDesc cc_tupdesc; + /* Needed to update indexes of cc_rel. */ ResultRelInfo *cc_rri; EState *cc_estate; @@ -199,6 +202,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 +2692,10 @@ apply_concurrent_changes(BufFile *file, ChangeContext *chgcxt) bool have_old_tuple = false; MemoryContext oldcxt; - spilled_tuple = MakeSingleTupleTableSlot(RelationGetDescr(rel), - &TTSOpsVirtual); + spilled_tuple = MakeSingleTupleTableSlot(chgcxt->cc_tupdesc, &TTSOpsVirtual); ondisk_tuple = MakeSingleTupleTableSlot(RelationGetDescr(rel), table_slot_callbacks(rel)); - old_update_tuple = MakeSingleTupleTableSlot(RelationGetDescr(rel), - &TTSOpsVirtual); + old_update_tuple = MakeSingleTupleTableSlot(chgcxt->cc_tupdesc, &TTSOpsVirtual); oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(chgcxt->cc_estate)); @@ -3156,10 +3158,19 @@ 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; + /* + * Use the descriptor of the source relation as the one to deform the + * decoded tuples with; in particular, this descriptor contains all the + * missing attributes. Tuples formed with it are also valid for the target + * relation, as the attributes are otherwise identical. + */ + chgcxt->cc_tupdesc = RelationGetDescr(src_relation); + /* Only initialize fields needed by ExecInsertIndexTuples(). */ chgcxt->cc_estate = CreateExecutorState(); @@ -3377,7 +3388,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 1c680abf7dd..136f0f77951 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -20,6 +20,7 @@ ISOLATION = basic \ repack \ repack_commit_race \ 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..351d76b3cbc --- /dev/null +++ b/src/test/modules/injection_points/expected/repack_missingval.out @@ -0,0 +1,38 @@ +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; + <waiting ...> +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 FROM repack_test ORDER BY i; + +i|j|c +-+-+--- +1|1|xyz +2|2|xyz +3|3|xyz +(3 rows) + +injection_points_detach +----------------------- + +(1 row) + diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 45117b6ffec..db6b93a8115 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_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..95995a2bd97 --- /dev/null +++ b/src/test/modules/injection_points/specs/repack_missingval.spec @@ -0,0 +1,83 @@ +# 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 value of "c" is only stored in + -- pg_attribute.attmissingval. + ALTER TABLE repack_test ADD COLUMN c text NOT NULL DEFAULT 'xyz'; + + -- With "c" in the replica identity, a missing value is also used as the key + -- to find the target tuple, not only to form the new row version. Use a + -- pass-by-reference type, because a NULL key of that kind can crash the + -- comparison function. + CREATE UNIQUE INDEX repack_test_ident_idx ON repack_test (i, c); + ALTER TABLE repack_test REPLICA IDENTITY USING INDEX repack_test_ident_idx; + + 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. +step s1_check +{ + SELECT i, j, c FROM repack_test ORDER BY i; +} +teardown +{ + SELECT injection_points_detach('repack-concurrently-before-lock'); +} + +session s2 + +# Update two of the three rows, without changing the data. +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'); +} + +permutation + s1_wait_before_lock + s2_update + s2_wakeup_before_lock + s1_check -- 2.47.3
