From f6f79e8fe1e54a387323206ba1394dbba3ed2366 Mon Sep 17 00:00:00 2001 From: Vignesh C Date: Fri, 4 Sep 2026 16:53:57 +0530 Subject: [PATCH v1] Lock tables against writers when altering their publication membership Publication DDL that names a table took only ShareUpdateExclusiveLock, which does not conflict with the RowExclusiveLock held by concurrent data-modifying statements. This allowed the publication definition to change while a statement was in progress. A data-modifying statement uses the publication definition from its catalog snapshot to decide whether the operation is allowed and what tuple data must be written to WAL. Logical decoding, however, uses the publication definition visible when the change is committed. If the publication definition changes in between, the publisher and decoder can use different definitions, leading to errors on the subscriber. Fix this by taking ShareRowExclusiveLock in OpenTableList() for publication DDL that changes table membership. This conflicts with RowExclusiveLock, preventing writers from running concurrently with the DDL, while still allowing readers to proceed. Also drop three subscriptions left behind by subscription/t/100_bugs.pl: sub_dropped_cols, sub1 and regress_sub. Their apply workers remain running when the shared publisher and subscriber are restarted, consuming all available logical replication worker slots and preventing the new test from starting its tablesync worker. --- src/backend/commands/publicationcmds.c | 24 +++-- src/test/subscription/t/100_bugs.pl | 125 +++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 96838730fe1..4243c9aa064 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -1398,7 +1398,7 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup, oldrel->columns = NIL; oldrel->except = false; oldrel->relation = table_open(oldrelid, - ShareUpdateExclusiveLock); + ShareRowExclusiveLock); delrels = lappend(delrels, oldrel); } } @@ -1831,8 +1831,20 @@ RemovePublicationSchemaById(Oid psoid) /* * Open relations specified by a PublicationTable list. - * The returned tables are locked in ShareUpdateExclusiveLock mode in order to - * add them to a publication. + * + * The returned tables are locked in ShareRowExclusiveLock mode in order to add + * them to a publication. The lock must conflict with RowExclusiveLock. Adding + * a table to a publication, or changing its row filter or column list, can + * affect both whether an UPDATE or DELETE is allowed and what tuple data is + * written to WAL. These decisions use the publication definition from the + * catalog snapshot taken at statement start and cannot be changed after the + * WAL is written. Logical decoding, however, uses the publication definition + * visible at commit time. Allowing this DDL to commit while a modification + * is in progress could therefore make WAL logging and logical decoding use + * different publication definitions. RowExclusiveLock is used by data + * modifications, so conflicting with it ensures that writers are blocked + * while the DDL runs and keeps the publication definition consistent for + * the entire modification. Readers are unaffected. */ static List * OpenTableList(List *tables) @@ -1857,7 +1869,7 @@ OpenTableList(List *tables) /* Allow query cancel in case this takes a long time */ CHECK_FOR_INTERRUPTS(); - rel = table_openrv(t->relation, ShareUpdateExclusiveLock); + rel = table_openrv(t->relation, ShareRowExclusiveLock); myrelid = RelationGetRelid(rel); /* @@ -1883,7 +1895,7 @@ OpenTableList(List *tables) errmsg("conflicting or redundant column lists for table \"%s\"", RelationGetRelationName(rel)))); - table_close(rel, ShareUpdateExclusiveLock); + table_close(rel, ShareRowExclusiveLock); continue; } @@ -1912,7 +1924,7 @@ OpenTableList(List *tables) List *children; ListCell *child; - children = find_all_inheritors(myrelid, ShareUpdateExclusiveLock, + children = find_all_inheritors(myrelid, ShareRowExclusiveLock, NULL); foreach(child, children) diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index 06c032a8e64..fabde35f4c4 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -425,6 +425,10 @@ is( $node_subscriber->safe_psql( qq(1), 'replication with RI FULL and dropped columns'); +# Clean up +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_dropped_cols"); +$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub_dropped_cols"); + $node_publisher->stop('fast'); $node_subscriber->stop('fast'); @@ -516,6 +520,10 @@ like( qr/ERROR: library "regress" may not be used as an output plugin/, 'loading unblessed output plugin fails: stderr'); +# Clean up +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1"); +$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1"); + $node_publisher->stop('fast'); $node_subscriber->stop('fast'); @@ -593,6 +601,10 @@ $result = $node_subscriber->safe_psql('postgres', is($result, 't', 'remote_lsn has advanced for apply worker raising an exception'); +# Clean up +$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION regress_sub"); +$node_publisher->safe_psql('postgres', "DROP PUBLICATION regress_pub"); + $node_publisher->stop('fast'); $node_subscriber->stop('fast'); @@ -759,4 +771,117 @@ DROP SUBSCRIPTION sub_drop_refresh; $node_publisher->stop('fast'); $node_subscriber->stop('fast'); +# The bug was caused by publication DDL taking ShareUpdateExclusiveLock, +# which does not conflict with the RowExclusiveLock held by a concurrent +# data-modifying statement. This allowed the publication definition to change +# while the statement was in progress. The publisher used the old definition +# when generating WAL, while logical decoding used the new definition, causing +# an UPDATE that should have been rejected on the publisher to fail only when +# applied on the subscriber. The published table has no replica identity, so +# the publisher sends no replica identity information and the subscriber +# cannot identify the row using its primary key. +# +# The bug is fixed by taking ShareRowExclusiveLock, which conflicts with +# RowExclusiveLock, so the ALTER PUBLICATION below waits for the in-progress +# UPDATE. The UPDATE is then decoded while the table is not yet part of any +# publication, so nothing is sent at all and the subscriber keeps the row it +# synced. +$node_publisher->rotate_logfile(); +$node_publisher->start(); + +$node_subscriber->rotate_logfile(); +$node_subscriber->start(); + +# An earlier block leaves this pointing at regress_db, and the block that +# resets it runs only when injection points are available. +$publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; + +$node_publisher->safe_psql( + 'postgres', qq{ + CREATE TABLE tab_pubrace (id int, val int); + INSERT INTO tab_pubrace VALUES (1, 1); + CREATE PUBLICATION pub_pubrace_sync FOR TABLE tab_pubrace; + CREATE PUBLICATION pub_pubrace_filtered; +}); + +$node_subscriber->safe_psql( + 'postgres', qq{ + CREATE TABLE tab_pubrace (id int PRIMARY KEY, val int); + CREATE SUBSCRIPTION sub_pubrace CONNECTION '$publisher_connstr' + PUBLICATION pub_pubrace_sync, pub_pubrace_filtered; +}); + +$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub_pubrace'); + +is( $node_subscriber->safe_psql( + 'postgres', 'SELECT id, val FROM tab_pubrace'), + '1|1', + 'initial sync copied the row before the concurrent publication DDL'); + +# Leave the table published by nothing, so that the racing DDL is what +# introduces the row filter. +$node_publisher->safe_psql('postgres', + 'ALTER PUBLICATION pub_pubrace_sync DROP TABLE tab_pubrace'); + +my $pubrace_offset = -s $node_subscriber->logfile; + +# Hold an UPDATE open. Its WAL record has already been written by the time the +# statement returns, which is all the window needs. +my $pubrace_dml = $node_publisher->background_psql('postgres'); +$pubrace_dml->query_safe( + "BEGIN;\nUPDATE tab_pubrace SET val = 2 WHERE id = 1;"); + +# Issue the DDL without waiting for it: it must not be able to commit while the +# UPDATE is in progres. A filter on val is invalid for UPDATE here, val being +# outside the replica identity. +my $pubrace_ddl = $node_publisher->background_psql('postgres'); +$pubrace_ddl->query_until( + qr/issued/, q{ + \echo issued + ALTER PUBLICATION pub_pubrace_filtered + ADD TABLE tab_pubrace WHERE (val = 2 OR val = 1); +}); + +ok( $node_publisher->poll_query_until( + 'postgres', qq{ + SELECT EXISTS (SELECT 1 FROM pg_locks + WHERE relation = 'tab_pubrace'::regclass + AND mode = 'ShareRowExclusiveLock' + AND NOT granted)}), + 'ALTER PUBLICATION waits for a concurrent data-modifying statement'); + +$pubrace_dml->query_safe('COMMIT'); +$pubrace_dml->quit; +$pubrace_ddl->query_until(qr/finished/, "\\echo finished\n"); +$pubrace_ddl->quit; + +$node_publisher->wait_for_catchup('sub_pubrace'); + +ok( !$node_subscriber->log_contains( + qr/publisher did not send replica identity column expected/, + $pubrace_offset), + 'no apply error from publication DDL racing an UPDATE'); + +# Verify that the UPDATE succeeded on the publisher. +is( $node_publisher->safe_psql( + 'postgres', 'SELECT id, val FROM tab_pubrace ORDER BY id'), + '1|2', + 'UPDATE committed on the publisher'); + +# The UPDATE is not sent because it was decoded before the table was added +# to the publication. +is( $node_subscriber->safe_psql( + 'postgres', 'SELECT id, val FROM tab_pubrace ORDER BY id'), + '1|1', + 'subscriber unchanged, the table being unpublished when the UPDATE committed' +); + +# Clean up +$node_subscriber->safe_psql('postgres', 'DROP SUBSCRIPTION sub_pubrace'); +$node_publisher->safe_psql('postgres', + 'DROP PUBLICATION pub_pubrace_sync, pub_pubrace_filtered'); + +$node_publisher->stop('fast'); +$node_subscriber->stop('fast'); + done_testing(); -- 2.55.0