From 207e5da69c74a2109753c63ddb97cc651f6abff8 Mon Sep 17 00:00:00 2001 From: nkey Date: Thu, 13 Aug 2026 11:11:52 +0200 Subject: [PATCH v2 1/4] 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 narrow am_partition to a partition that still has one. It is then published in its own right, 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. Author: Mikhail Nikalayeu Reported-by: Nisha Moond Reported-by: Zsolt Parragi Reviewed-by: Álvaro Herrera Reviewed-by: Nisha Moond Reviewed-by: shveta malik Reviewed-by: Amit Kapila 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: 14 --- src/backend/catalog/partition.c | 5 + src/backend/replication/pgoutput/pgoutput.c | 17 ++- src/test/subscription/t/100_bugs.pl | 110 ++++++++++++++++++++ 3 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 28f3cade6ff..d2bb3d3498b 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 b481d5bb388..eb965ecbd5a 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2107,6 +2107,20 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) 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 (am_partition) + { + ancestors = get_partition_ancestors(relid); + am_partition = (ancestors != NIL); + } /* Reload publications if needed before use. */ if (!publications_valid) @@ -2214,7 +2228,6 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) if (am_partition) { - List *ancestors = get_partition_ancestors(relid); Oid last_ancestor_relid = llast_oid(ancestors); /* @@ -2267,7 +2280,6 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) { Oid ancestor; int level; - List *ancestors = get_partition_ancestors(relid); ancestor = GetTopMostAncestorInPublication(pub->oid, ancestors, @@ -2365,6 +2377,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; } diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 06c032a8e64..7f125e49fc1 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -79,6 +79,116 @@ $node_publisher->stop('fast'); $node_subscriber->stop('fast'); +# Replicating changes to a partition whose concurrent detach never finished. + +# Between the two transactions of ALTER TABLE ... DETACH PARTITION ... +# CONCURRENTLY, pg_class still says relispartition while +# get_partition_ancestors() already reports nothing, and get_rel_sync_entry() +# crashed on that; for a subscription that is a crash loop. The state outlives +# the command: only DETACH PARTITION ... FINALIZE clears it. + +$node_publisher->append_conf('postgresql.conf', + 'max_prepared_transactions = 1'); +$node_publisher->rotate_logfile(); +$node_publisher->start(); +$node_subscriber->rotate_logfile(); +$node_subscriber->start(); + +my $detach_ddl = q[ + CREATE TABLE parted (a int, b int) PARTITION BY LIST (a); + CREATE TABLE part1 PARTITION OF parted FOR VALUES IN (1); + CREATE TABLE part2 PARTITION OF parted FOR VALUES IN (2); +]; +$node_publisher->safe_psql('postgres', $detach_ddl); +$node_subscriber->safe_psql('postgres', $detach_ddl); + +$node_publisher->safe_psql( + 'postgres', q[ + CREATE PUBLICATION pub_detach FOR ALL TABLES + WITH (publish_via_partition_root = true); +]); +$node_subscriber->safe_psql('postgres', + "CREATE SUBSCRIPTION sub_detach CONNECTION '$publisher_connstr' PUBLICATION pub_detach" +); +$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub_detach'); + +$node_publisher->safe_psql('postgres', + 'INSERT INTO parted VALUES (1, 1), (2, 1)'); +$node_publisher->wait_for_catchup('sub_detach'); + +is( $node_subscriber->safe_psql( + 'postgres', 'SELECT * FROM parted ORDER BY a, b'), + "1|1\n2|1", + 'both partitions replicate while attached'); + +# Leave part1 pending detach: a prepared transaction holds the snapshot the +# detach waits for, so the wait ends in the lock timeout. +$node_publisher->safe_psql('postgres', + q[BEGIN; SELECT count(*) FROM parted; PREPARE TRANSACTION 'holder';]); +{ + local $ENV{PGOPTIONS} = '-c lock_timeout=1s'; + $node_publisher->psql( + 'postgres', + 'ALTER TABLE parted DETACH PARTITION part1 CONCURRENTLY', + on_error_stop => 0); +} +$node_publisher->safe_psql('postgres', q[ROLLBACK PREPARED 'holder']); + +is( $node_publisher->safe_psql( + 'postgres', q[ + SELECT inhdetachpending FROM pg_inherits + WHERE inhrelid = 'part1'::regclass]), + 't', + 'the partition is marked as detaching'); + +# Changes nothing; drops the walsender's cached mapping. +$node_publisher->safe_psql('postgres', + 'ALTER PUBLICATION pub_detach SET (publish_via_partition_root = true)'); + +# Use another slot to check which changes pgoutput sends, independently of +# whether the subscriber applies them. +$node_publisher->safe_psql('postgres', + q[SELECT pg_create_logical_replication_slot('detach_slot', 'pgoutput')]); + +# This is what crashed. The part1 change is sent using part1's identity, but +# the subscriber skips it because only parted is registered in +# pg_subscription_rel. The following change shows decoding got past it. +$node_publisher->safe_psql( + 'postgres', q[ + INSERT INTO part1 VALUES (1, 2); + INSERT INTO parted VALUES (2, 2); +]); +$node_publisher->wait_for_catchup('sub_detach'); + +is( $node_subscriber->safe_psql( + 'postgres', 'SELECT * FROM parted ORDER BY a, b'), + "1|1\n2|1\n2|2", + 'replication got past the partition pending detach'); + +# In protocol version 1, an Insert message starts with 'I' followed by the +# relation OID. Verify that pgoutput sends the change using part1's identity. +is( $node_publisher->safe_psql( + 'postgres', q[ + SELECT count(*) + FROM pg_logical_slot_peek_binary_changes('detach_slot', NULL, NULL, + 'proto_version', '1', 'publication_names', 'pub_detach') + WHERE get_byte(data, 0) = ascii('I') + AND substring(data FROM 2 FOR 4) = int4send('part1'::regclass::oid::int)]), + '1', + 'the change is sent as the partition pending detach'); +$node_publisher->safe_psql('postgres', + q[SELECT pg_drop_replication_slot('detach_slot')]); + +# Drop replication state and the tables, as the tests below re-use the nodes. +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_detach"); +$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub_detach"); +$node_publisher->safe_psql('postgres', "DROP TABLE parted, part1"); +$node_subscriber->safe_psql('postgres', "DROP TABLE parted"); + +$node_publisher->stop('fast'); +$node_subscriber->stop('fast'); + + # Handling of temporary and unlogged tables with FOR ALL TABLES publications # If a FOR ALL TABLES publication exists, temporary and unlogged -- 2.43.0