From e4b367d6c8c476b20dc54599d5fd1beab5707e02 Mon Sep 17 00:00:00 2001 From: Mikhail Nikalayeu Date: Wed, 11 Nov 2026 19:21:58 +0800 Subject: [PATCH v3] Fix crash decoding a change to a partition pending detach. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ALTER TABLE ... DETACH PARTITION ... CONCURRENTLY, added by 71f4c8c6f74b, leaves the partition marked as detaching when its wait is interrupted -- by a lock timeout, a cancel, a disconnect. Only DETACH PARTITION ... FINALIZE clears that mark, and the partition cannot be attached back. In that state pg_class still says relispartition, while get_partition_ancestors() already reports nothing. get_rel_sync_entry() was not ready for it: it read relispartition as meaning that the ancestor list is not empty, and asked for its last element, which is an assertion failure, and a NULL pointer dereference without assertions. Decoding any change to such a partition for a FOR ALL TABLES publication gets there, and as the change is never confirmed, a subscription turns that into a crash loop. Fetch the ancestors once, up front, and treat a partition with no ancestors as a standalone table, which is how it is published once the detach completes. The crash became reachable in v14, where 71f4c8c6f74b introduced the state, and fd366065e06a widened it from a publication with publish_via_partition_root to any FOR ALL TABLES one. Reported-by: Mikhail Nikalayeu Reported-by: Nisha Moond Reported-by: Zsolt Parragi Author: Mikhail Nikalayeu Author: Zhijie Hou Reviewed-by: Álvaro Herrera Reviewed-by: Nisha Moond Reviewed-by: shveta malik Reviewed-by: Amit Kapila Reviewed-by: Masahiko Sawada Reviewed-by: Zhijie Hou Discussion: https://postgr.es/m/CADzfLwWoFPT%2Ba73%3DA%3DbsNWRMZQ98NpBEMgE%3Dt1FS4O4_%3DQVLfA%40mail.gmail.com Discussion: https://postgr.es/m/CABdArM64ZSat%3DdB094U8OCBzp1m8_Y85m6T%2BxPEgpLVJb60_rA%40mail.gmail.com Backpatch-through: 15 --- .../expected/concurrent_ddl_dml.out | 38 +++++++++++++++++++ .../specs/concurrent_ddl_dml.spec | 17 +++++++++ src/backend/catalog/partition.c | 5 +++ src/backend/replication/pgoutput/pgoutput.c | 20 +++++++--- 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/contrib/test_decoding/expected/concurrent_ddl_dml.out b/contrib/test_decoding/expected/concurrent_ddl_dml.out index 3742a2a2474..a09776fea8a 100644 --- a/contrib/test_decoding/expected/concurrent_ddl_dml.out +++ b/contrib/test_decoding/expected/concurrent_ddl_dml.out @@ -797,3 +797,41 @@ COMMIT stop (1 row) + +starting permutation: s1_init_pgoutput s1_begin s1_select_part s2_detach_part s1_cancel_s2 s1_commit s1_create_pub s1_insert_part s2_get_binary_changes +step s1_init_pgoutput: SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'pgoutput'); +?column? +-------- +init +(1 row) + +step s1_begin: BEGIN; +step s1_select_part: SELECT * FROM tbl_part; +val1 +---- +(0 rows) + +step s2_detach_part: ALTER TABLE tbl_part DETACH PARTITION tbl_part_p1 CONCURRENTLY; +step s1_cancel_s2: SELECT pg_cancel_backend(pid) FROM pg_stat_activity WHERE application_name = 'isolation/concurrent_ddl_dml/s2'; +step s2_detach_part: <... completed> +ERROR: canceling statement due to user request +step s1_cancel_s2: <... completed> +pg_cancel_backend +----------------- +t +(1 row) + +step s1_commit: COMMIT; +step s1_create_pub: CREATE PUBLICATION pub FOR ALL TABLES WITH (publish_via_partition_root = true); +step s1_insert_part: INSERT INTO tbl_part_p1 VALUES (1); +step s2_get_binary_changes: SELECT count(*) > 0 FROM pg_logical_slot_get_binary_changes('isolation_slot', NULL, NULL, 'proto_version', '1', 'publication_names', 'pub'); +?column? +-------- +t +(1 row) + +?column? +-------- +stop +(1 row) + diff --git a/contrib/test_decoding/specs/concurrent_ddl_dml.spec b/contrib/test_decoding/specs/concurrent_ddl_dml.spec index d16515e6f48..77ec8c133a5 100644 --- a/contrib/test_decoding/specs/concurrent_ddl_dml.spec +++ b/contrib/test_decoding/specs/concurrent_ddl_dml.spec @@ -2,12 +2,17 @@ setup { DROP TABLE IF EXISTS tbl1; DROP TABLE IF EXISTS tbl2; + DROP TABLE IF EXISTS tbl_part, tbl_part_p1; CREATE TABLE tbl1(val1 integer, val2 integer); CREATE TABLE tbl2(val1 integer, val2 integer); + CREATE TABLE tbl_part(val1 integer) PARTITION BY LIST (val1); + CREATE TABLE tbl_part_p1 PARTITION OF tbl_part FOR VALUES IN (1); } teardown { + DROP PUBLICATION IF EXISTS pub; + DROP TABLE IF EXISTS tbl_part, tbl_part_p1; DROP TABLE tbl1; DROP TABLE tbl2; SELECT 'stop' FROM pg_drop_replication_slot('isolation_slot'); @@ -17,10 +22,15 @@ session "s1" setup { SET synchronous_commit=on; } step "s1_init" { SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'test_decoding'); } +step "s1_init_pgoutput" { SELECT 'init' FROM pg_create_logical_replication_slot('isolation_slot', 'pgoutput'); } step "s1_begin" { BEGIN; } step "s1_insert_tbl1" { INSERT INTO tbl1 (val1, val2) VALUES (1, 1); } step "s1_insert_tbl2" { INSERT INTO tbl2 (val1, val2) VALUES (1, 1); } step "s1_insert_tbl2_3col" { INSERT INTO tbl2 (val1, val2, val3) VALUES (1, 1, 1); } +step "s1_select_part" { SELECT * FROM tbl_part; } +step "s1_cancel_s2" { SELECT pg_cancel_backend(pid) FROM pg_stat_activity WHERE application_name = 'isolation/concurrent_ddl_dml/s2'; } +step "s1_create_pub" { CREATE PUBLICATION pub FOR ALL TABLES WITH (publish_via_partition_root = true); } +step "s1_insert_part" { INSERT INTO tbl_part_p1 VALUES (1); } step "s1_commit" { COMMIT; } session "s2" @@ -43,8 +53,10 @@ step "s2_alter_tbl2_drop_3rd_col" { ALTER TABLE tbl2 DROP COLUMN val3; } step "s2_alter_tbl2_3rd_char" { ALTER TABLE tbl2 ALTER COLUMN val3 TYPE character varying; } step "s2_alter_tbl2_3rd_text" { ALTER TABLE tbl2 ALTER COLUMN val3 TYPE text; } step "s2_alter_tbl2_3rd_int" { ALTER TABLE tbl2 ALTER COLUMN val3 TYPE int USING val3::integer; } +step "s2_detach_part" { ALTER TABLE tbl_part DETACH PARTITION tbl_part_p1 CONCURRENTLY; } step "s2_get_changes" { SELECT data FROM pg_logical_slot_get_changes('isolation_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1'); } +step "s2_get_binary_changes" { SELECT count(*) > 0 FROM pg_logical_slot_get_binary_changes('isolation_slot', NULL, NULL, 'proto_version', '1', 'publication_names', 'pub'); } @@ -86,3 +98,8 @@ permutation "s1_init" "s2_alter_tbl2_add_char" "s1_begin" "s1_insert_tbl1" "s2_a permutation "s1_init" "s2_alter_tbl2_add_text" "s1_begin" "s1_insert_tbl1" "s2_alter_tbl2_3rd_char" "s1_insert_tbl2_3col" "s1_commit" "s2_alter_tbl2_drop_3rd_col" "s1_insert_tbl2" "s2_get_changes" permutation "s1_init" "s2_alter_tbl2_add_char" "s1_begin" "s1_insert_tbl1" "s2_alter_tbl2_drop_3rd_col" "s1_insert_tbl1" "s1_commit" "s2_get_changes" + +# Verify that pgoutput can decode changes to a partition whose concurrent +# detach was canceled, when the relation is still marked as a partition in +# pg_class but already reports no ancestors. +permutation "s1_init_pgoutput" "s1_begin" "s1_select_part" "s2_detach_part" "s1_cancel_s2"("s2_detach_part") "s1_commit" "s1_create_pub" "s1_insert_part" "s2_get_binary_changes" diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 93d72157a46..c8e116bd1fb 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -126,6 +126,11 @@ get_partition_parent_worker(Relation inhRel, Oid relid, bool *detach_pending) * The first element is the immediate parent and the last one is the topmost * parent in the partition hierarchy. * + * A partition pending concurrent detach is treated as having no parent. The + * list can therefore be empty even if the relation is still marked as a + * partition in pg_class. If an ancestor is pending detach, the list ends at + * that ancestor. + * * Note: Because this function assumes that the relation whose OID is passed * as an argument and each ancestor will have precisely one parent, it should * only be called when it is known that the relation is a partition. diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index 7eed467edd5..e9781da17b1 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2100,9 +2100,19 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) ListCell *lc; Oid publish_as_relid = relid; int publish_ancestor_level = 0; - bool am_partition = get_rel_relispartition(relid); char relkind = get_rel_relkind(relid); List *rel_publications = NIL; + List *ancestors = NIL; + + /* + * A partition whose concurrent detach has been committed but not + * finalized reports no ancestors, even though relispartition is still + * set. Determine publication membership and identity as for a + * standalone table: changes are no longer published via its former + * ancestors, but can still be published via its own publications. + */ + if (get_rel_relispartition(relid)) + ancestors = get_partition_ancestors(relid); /* Reload publications if needed before use. */ if (!publications_valid) @@ -2207,10 +2217,8 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) if (pub->alltables) { publish = true; - if (pub->pubviaroot && am_partition) + if (pub->pubviaroot && ancestors) { - List *ancestors = get_partition_ancestors(relid); - pub_relid = llast_oid(ancestors); ancestor_level = list_length(ancestors); } @@ -2226,11 +2234,10 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) * published via this publication, which will be used as the * relation via which to publish the partition's changes. */ - if (am_partition) + if (ancestors) { Oid ancestor; int level; - List *ancestors = get_partition_ancestors(relid); ancestor = GetTopMostAncestorInPublication(pub->oid, ancestors, @@ -2328,6 +2335,7 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) list_free(pubids); list_free(schemaPubids); list_free(rel_publications); + list_free(ancestors); entry->replicate_valid = true; } -- 2.54.0 (Apple Git-157)