Hi, While reviewing another logical replication thread at [1], I noticed a race between an update and a concurrent publication DDL that can result in an invalid logical replication change being sent to the subscriber. The following reproduces the issue: -- publisher CREATE TABLE tab_nonri (id int, val int); CREATE PUBLICATION pub_sync FOR TABLE tab_nonri; CREATE PUBLICATION pub_filtered; INSERT INTO tab_nonri VALUES (1, 1); -- subscriber CREATE TABLE tab_nonri (id int PRIMARY KEY, val int); CREATE SUBSCRIPTION ... PUBLICATION pub_sync, pub_filtered; -- publisher ALTER PUBLICATION pub_sync DROP TABLE tab_nonri; -- publisher session 1 BEGIN; UPDATE tab_nonri SET val = 2 WHERE id = 1; -- hold debugger at the end of CheckCmdReplicaIdentity() function
-- publisher session 2 ALTER PUBLICATION pub_filtered ADD TABLE tab_nonri WHERE (val = 2 OR val = 1); -- publisher session 1 -- continue the debugger COMMIT; The UPDATE is checked using the publication definition from the catalog snapshot taken at the start of the statement, before tab_nonri is added to pub_filtered. Therefore, the UPDATE is allowed to proceed. However, by the time the change is decoded, the new publication definition is visible. The publisher consequently sends the change as: old tuple = <absent> new tuple = (id = 1, val = 2) Since tab_nonri has no replica identity on the publisher, no replica identity information is sent. On the subscriber, tab_nonri has a primary key and therefore a local replica identity index (tab_nonri_pkey). However, there are no publisher replica identity columns to match against, so remoterel->attkeys is NULL and the relation is marked as not updatable. The subscriber eventually reports: ERROR: publisher did not send replica identity column expected by the logical replication target relation "public.tab_nonri" After this, the apply worker repeatedly fails with the same error when it tries to apply the change. With proper locking, this should instead be handled on the publisher: if the publication has already added the table when the UPDATE is executed, the UPDATE should be rejected with: ERROR: cannot update table "tab_nonri" DETAIL: Column used in the publication WHERE expression is not part of the replica identity. If the update is in progress, then ALTER PUBLICATION should wait for the UPDATE to complete. The underlying issue appears to be that publication DDL naming a table takes ShareUpdateExclusiveLock, which does not conflict with the RowExclusiveLock held by a concurrent UPDATE. This allows the publication definition to change while the UPDATE is in progress: the UPDATE uses the old definition when checking the operation and generating WAL, while logical decoding uses the new definition. There may be other variations of this race ex: adding column list, but the underlying problem is the same: an UPDATE can proceed based on stale publication information and produce a logical replication change that is rejected only later on the subscriber. I fixed the issue by changing the lock taken by publication DDL from ShareUpdateExclusiveLock to ShareRowExclusiveLock. This makes the DDL conflict with the RowExclusiveLock held by concurrent data-modifying statements, preventing the publication definition from changing while the UPDATE is in progress. [1] - https://www.postgresql.org/message-id/CAOzEurQaGBrDu8hFw%2Bp16_uf_%2Bb94A276fYgzwUOK5X-bSX%2Bjg%40mail.gmail.com Regards, Vignesh
v1-0001-Lock-tables-against-writers-when-altering-their-p.patch
Description: Binary data
