Publication DDL can race with a concurrent UPDATE

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