From 21de09d8b6e5814555738932fe6f3e615ce3eea7 Mon Sep 17 00:00:00 2001 From: Zhijie Hou Date: Wed, 11 Nov 2026 15:30:40 +0800 Subject: [PATCH] Top-up for v2-0001: Simplify am_partition check and test in isolation. In get_rel_sync_entry(), remove the boolean am_partition variable and check the ancestors list directly, matching RelationBuildPublicationDesc() in relcache.c. Also, replace the new subscription TAP test in 100_bugs.pl with a decoding step in the existing detach-partition-concurrently-3 isolation permutation. Since this patch only touches pgoutput decoding on the publisher side, testing via pg_logical_slot_get_binary_changes() in the existing isolation permutation exercises the exact code path without the overhead of setting up a subscriber and restarting nodes. Author: Zhijie Hou Discussion: https://postgr.es/m/CADzfLwWoFPT%2Ba73%3DA%3DbsNWRMZQ98NpBEMgE%3Dt1FS4O4_%3DQVLfA%40mail.gmail.com --- src/backend/replication/pgoutput/pgoutput.c | 10 +- .../detach-partition-concurrently-3.out | 18 ++- .../detach-partition-concurrently-3.spec | 17 ++- src/test/subscription/t/100_bugs.pl | 110 ------------------ 4 files changed, 31 insertions(+), 124 deletions(-) diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c index e74a9f156e8..b5fb5ead85a 100644 --- a/src/backend/replication/pgoutput/pgoutput.c +++ b/src/backend/replication/pgoutput/pgoutput.c @@ -2103,7 +2103,6 @@ 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; @@ -2115,11 +2114,8 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) * standalone table: changes are no longer published via its former * ancestors, but can still be published via its own publications. */ - if (am_partition) - { + if (get_rel_relispartition(relid)) ancestors = get_partition_ancestors(relid); - am_partition = (ancestors != NIL); - } /* Reload publications if needed before use. */ if (!publications_valid) @@ -2225,7 +2221,7 @@ get_rel_sync_entry(PGOutputData *data, Relation relation) { List *exceptpubids = NIL; - if (am_partition) + if (ancestors) { Oid last_ancestor_relid = llast_oid(ancestors); @@ -2275,7 +2271,7 @@ 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; diff --git a/src/test/isolation/expected/detach-partition-concurrently-3.out b/src/test/isolation/expected/detach-partition-concurrently-3.out index c1705d80aa1..75ff86b1701 100644 --- a/src/test/isolation/expected/detach-partition-concurrently-3.out +++ b/src/test/isolation/expected/detach-partition-concurrently-3.out @@ -106,7 +106,7 @@ t step s1c: COMMIT; step s1insertpart: INSERT INTO d3_listp1 VALUES (1); -starting permutation: s2snitch s1b s1s s2detach s1cancel s1c s1updpart s1delpart s1exceptpart +starting permutation: s2snitch s1b s1s s2detach s1cancel s1c s1updpart s1delpart s1exceptpart s1initslot s1insertpart s1decodepart step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); step s1b: BEGIN; step s1s: SELECT * FROM d3_listp; @@ -130,6 +130,22 @@ step s1updpart: UPDATE d3_listp1 SET a = 1; step s1delpart: DELETE FROM d3_listp1; step s1exceptpart: CREATE PUBLICATION pub_d3 FOR ALL TABLES EXCEPT (TABLE d3_listp1); ERROR: cannot specify relation "public.d3_listp1" in the publication EXCEPT clause +step s1initslot: SELECT 'init' FROM pg_create_logical_replication_slot('d3_slot', 'pgoutput', true); + CREATE PUBLICATION pub_d3 FOR ALL TABLES WITH (publish_via_partition_root = true); +?column? +-------- +init +(1 row) + +step s1insertpart: INSERT INTO d3_listp1 VALUES (1); +step s1decodepart: SELECT count(*) > 0 FROM pg_logical_slot_get_binary_changes('d3_slot', NULL, NULL, + 'proto_version', '1', 'publication_names', 'pub_d3'); + DROP PUBLICATION pub_d3; +?column? +-------- +t +(1 row) + starting permutation: s2snitch s1b s1s s2detach2 s1cancel s1c s1brr s1insert s1s s1insert s1c step s2snitch: INSERT INTO d3_pid SELECT pg_backend_pid(); diff --git a/src/test/isolation/specs/detach-partition-concurrently-3.spec b/src/test/isolation/specs/detach-partition-concurrently-3.spec index 6d359756e31..1e9cd66d52b 100644 --- a/src/test/isolation/specs/detach-partition-concurrently-3.spec +++ b/src/test/isolation/specs/detach-partition-concurrently-3.spec @@ -34,6 +34,11 @@ step s1insertpart { INSERT INTO d3_listp1 VALUES (1); } step s1updpart { UPDATE d3_listp1 SET a = 1; } step s1delpart { DELETE FROM d3_listp1; } step s1exceptpart { CREATE PUBLICATION pub_d3 FOR ALL TABLES EXCEPT (TABLE d3_listp1); } +step s1initslot { SELECT 'init' FROM pg_create_logical_replication_slot('d3_slot', 'pgoutput', true); + CREATE PUBLICATION pub_d3 FOR ALL TABLES WITH (publish_via_partition_root = true); } +step s1decodepart { SELECT count(*) > 0 FROM pg_logical_slot_get_binary_changes('d3_slot', NULL, NULL, + 'proto_version', '1', 'publication_names', 'pub_d3'); + DROP PUBLICATION pub_d3; } step s1drop { DROP TABLE d3_listp; } step s1droppart { DROP TABLE d3_listp1; } step s1trunc { TRUNCATE TABLE d3_listp; } @@ -58,12 +63,12 @@ permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1insert s1c permutation s2snitch s1brr s1s s2detach s1cancel(s2detach) s1insert s1c s1spart permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1c s1insertpart -# Verify that UPDATE, DELETE, and publication exclusion addition -# using EXCEPT clause correctly handle a concurrently detached partition. -# A partition whose concurrent detach has been committed but not finalized -# reports no ancestors, even though relispartition is still set, which -# would otherwise trigger a topmost ancestor lookup crash. -permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1c s1updpart s1delpart s1exceptpart +# Verify that UPDATE, DELETE, publication exclusion addition using EXCEPT +# clause, and pgoutput decoding correctly handle a concurrently detached +# partition. A partition whose concurrent detach has been committed but not +# finalized reports no ancestors, even though relispartition is still set, +# which would otherwise trigger a topmost ancestor lookup crash. +permutation s2snitch s1b s1s s2detach s1cancel(s2detach) s1c s1updpart s1delpart s1exceptpart s1initslot s1insertpart s1decodepart # Test partition descriptor caching permutation s2snitch s1b s1s s2detach2 s1cancel(s2detach2) s1c s1brr s1insert s1s s1insert s1c diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 6a21b188b4b..a39d6b0d100 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -79,116 +79,6 @@ $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.54.0 (Apple Git-157)