Publication DDL can race with a concurrent UPDATE
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253699psql -h localhost -U postgresThis image is from patchset v11 (message #11) - the current patchset v13 (message #13) has not produced an image.
Hi,
While reviewing another logical replication thread at [1]/messages/by-id/CAOzEurQaGBrDu8hFw+p16_uf_+b94A276fYgzwUOK5X-bSX+jg@mail.gmail.com, 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]: /messages/by-id/CAOzEurQaGBrDu8hFw+p16_uf_+b94A276fYgzwUOK5X-bSX+jg@mail.gmail.com
Regards,
Vignesh
On Monday, September 7, 2026 8:56 PM vignesh C <vignesh21@gmail.com> wrote:
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.
Thanks for reporting the issue.
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.
I think this fix is not sufficient, as it does not address the ALTER PUBLICATION
SET (options) cases, where the publication action can also be altered
concurrently with DMLs, IIUC. The publication data in the relcache is also
affected by pubaction changes, so those should be blocked as well.
Addressing the above should be sufficient for the row filter and column list
cases. However, for the replica identity check on UPDATE and DELETE operations,
further analysis may be needed - especially for the TABLES IN SCHEMA and ALL
TABLES cases, where tables are not explicitly published.
[1] - https://www.postgresql.org/message-
id/CAOzEurQaGBrDu8hFw%2Bp16_uf_%2Bb94A276fYgzwUOK5X-
bSX%2Bjg%40mail.gmail.com
Best Regards,
Zhijie Hou
On Tue, 8 Sept 2026 at 10:54, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
On Monday, September 7, 2026 8:56 PM vignesh C <vignesh21@gmail.com> wrote:
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.Thanks for reporting the issue.
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.I think this fix is not sufficient, as it does not address the ALTER PUBLICATION
SET (options) cases, where the publication action can also be altered
concurrently with DMLs, IIUC. The publication data in the relcache is also
affected by pubaction changes, so those should be blocked as well.Addressing the above should be sufficient for the row filter and column list
cases. However, for the replica identity check on UPDATE and DELETE operations,
further analysis may be needed - especially for the TABLES IN SCHEMA and ALL
TABLES cases, where tables are not explicitly published.
One approach could be:
For "TABLES IN SCHEMA", lock the schema's namespace OID.
LockSchemaList() already takes a lock on the schema to prevent DROP
SCHEMA, so upgrade it from AccessShareLock to ShareRowExclusiveLock.
This keeps the existing protection and also prevents concurrent
writers from racing with CREATE PUBLICATION ... FOR TABLES IN SCHEMA
and ALTER PUBLICATION ... ADD/SET TABLES IN SCHEMA. For "FOR ALL
TABLES", lock the pg_publication relation with ShareRowExclusiveLock.
Take this lock in CreatePublication() when enabling FOR ALL TABLES,
and in AlterPublicationAllFlags() when changing puballtables from
false to true. On the DML side(UPDATE and DELETE),
CheckCmdReplicaIdentity() takes a matching RowExclusiveLock on the
table's namespace and pg_publication before using the publication
descriptor. This is needed only for tables without a local replica
identity; tables with a replica identity or REPLICA IDENTITY FULL are
not affected by this race.
RowExclusiveLock is self-compatible, so normal concurrent DML does not
block other DML. It conflicts with the ShareRowExclusiveLock taken by
the publication DDL, ensuring that the DDL and DML cannot race.
The attached POC demonstrates the changes for the same.
Does this approach look reasonable, or is there a better way to handle
this synchronization?
Regards,
Vignesh
On Tue, 29 Sept 2026 at 15:41, vignesh C <vignesh21@gmail.com> wrote:
The attached POC demonstrates the changes for the same.
Here is the updated v2 patch, which addresses Hou-san's comment at [1]/messages/by-id/TY4PR01MB17718FD116A2C749F35D28FD094B12@TY4PR01MB17718.jpnprd01.prod.outlook.com
regarding concurrent UPDATE/DELETE with ALTER PUBLICATION SET
(options).
[1]: /messages/by-id/TY4PR01MB17718FD116A2C749F35D28FD094B12@TY4PR01MB17718.jpnprd01.prod.outlook.com
Regards,
Vignesh
Attachments:
t253699_4v2-0002-Concurrent-UPDATE-DELETE-with-the-following-CREAT.patchapplication/octet-stream; name=v2-0002-Concurrent-UPDATE-DELETE-with-the-following-CREAT.patchDownload+297-16
v2-0001-Lock-tables-against-writers-when-altering-their-p.patchapplication/octet-stream; name=v2-0001-Lock-tables-against-writers-when-altering-their-p.patchDownload+143-7
Hi,
On Tue, Sep 29, 2026 at 6:12 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 8 Sept 2026 at 10:54, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:I think this fix is not sufficient, as it does not address the ALTER PUBLICATION
SET (options) cases, where the publication action can also be altered
concurrently with DMLs, IIUC. The publication data in the relcache is also
affected by pubaction changes, so those should be blocked as well.Addressing the above should be sufficient for the row filter and column list
cases. However, for the replica identity check on UPDATE and DELETE operations,
further analysis may be needed - especially for the TABLES IN SCHEMA and ALL
TABLES cases, where tables are not explicitly published.One approach could be:
For "TABLES IN SCHEMA", lock the schema's namespace OID.
LockSchemaList() already takes a lock on the schema to prevent DROP
SCHEMA, so upgrade it from AccessShareLock to ShareRowExclusiveLock.
This keeps the existing protection and also prevents concurrent
writers from racing with CREATE PUBLICATION ... FOR TABLES IN SCHEMA
and ALTER PUBLICATION ... ADD/SET TABLES IN SCHEMA. For "FOR ALL
TABLES", lock the pg_publication relation with ShareRowExclusiveLock.
Take this lock in CreatePublication() when enabling FOR ALL TABLES,
and in AlterPublicationAllFlags() when changing puballtables from
false to true. On the DML side(UPDATE and DELETE),
CheckCmdReplicaIdentity() takes a matching RowExclusiveLock on the
table's namespace and pg_publication before using the publication
descriptor. This is needed only for tables without a local replica
identity; tables with a replica identity or REPLICA IDENTITY FULL are
not affected by this race.
RowExclusiveLock is self-compatible, so normal concurrent DML does not
block other DML. It conflicts with the ShareRowExclusiveLock taken by
the publication DDL, ensuring that the DDL and DML cannot race.
The attached POC demonstrates the changes for the same.Does this approach look reasonable, or is there a better way to handle
this synchronization?
After considering more...
I think adding a heavy lock acquisition per DML statement sounds risky,
especially since this is a hot path that every table lacking a replica identity
will go through, whether or not it's published. Acquiring a lock even when no
conflict exists is not free either and the lock is held until transaction end.
I recall an argument [1]/messages/by-id/CA+TgmoZzoLzcbrP9=Y3E=6xfyp=uH8CobowhuDV8fLbisQtbyA@mail.gmail.com in another thread that adding an additional
heavyweight lock per transaction could limit the maximum achievable TPS. And
considering it's not rare to run pgbench with many backends doing UPDATE/DELETE
on tables lacking a replica identity, this needs more thoughts.
I'm trying to explore whether we can narrow the scope to the DDL side as much
as possible, to avoid adding such a risky change to the hot path.
Here is one idea, it's not perfect but I feel it's worth discussing.
The idea is to make TABLES IN SCHEMA or ALL TABLES publications lock the tables
only when they have neither a replica identity index nor replica identity FULL,
to prevent concurrent DML on these tables to execute. We only lock such tables,
because if we allow DML to write logs on these tables, the changes cannot be
replayed on the subscriber side due to lack of identity key. Nothing is locked
when the publication does not publish UPDATE or DELETE, since only those
actions require a replica identity. For tables named explicitly in a
publication, we increase the lock level as Vignesh's patch does, and
additionally lock their partition trees, the explicitly listed members when SET
(publish = ...) enables UPDATE or DELETE, and the tree of a dropped EXCEPT
exclusion.
For new tables created concurrently, we can take a conflicting lock on the
publication catalog only when building the pubcache in
RelationBuildPublicationDesc(). The descriptor is built exactly when the
relcache entry is fresh or was invalidated, which means it's possible the table
was just created. The DDL holds the catalog lock until commit, so it cannot
commit between the descriptor build and the WAL write. This is the only lock
taken on the DML side, and steady-state DML never touches it. This also
protects the cases of tables that had a replica identity: if the identity index
is removed from the table, the cache will be rebuilt and the writer will take
the catalog lock, serializing with a concurrent publication DDL that may not
have seen the removal.
In the worst case this could still lock a large number of tables, so the DDL
counts them and raises a clear error ("too many tables without a replica
identity in the publication") instead of a later "out of shared memory". This
is expected to be very rare, and since UPDATEs/DELETEs on those tables fail
anyway, the error should be acceptable because it hints that the publication
contains many tables that can't actually be published (since they lack RI).
I'm sharing two POC patches for reference and discussion (they're not in great
shape and might have some bugs, but they basically implement the idea above).
0001 simply increases the lock level for explicitly included tables (FOR TABLE
a, b) in a publication, and additionally locks the child table when only the
parent is published. 0002 handles the TABLES IN SCHEMA and ALL TABLES cases,
locking tables lacking RI as described above.
[1]: /messages/by-id/CA+TgmoZzoLzcbrP9=Y3E=6xfyp=uH8CobowhuDV8fLbisQtbyA@mail.gmail.com
Best Regards,
Zhijie Hou
Attachments:
t253699_5vPOC-0001-Lock-tables-against-writers-when-altering-publi.patchapplication/octet-stream; name=vPOC-0001-Lock-tables-against-writers-when-altering-publi.patchDownload+227-14
vPOC-0002-Synchronize-publication-scope-widening-with-con.patchapplication/octet-stream; name=vPOC-0002-Synchronize-publication-scope-widening-with-con.patchDownload+457-2
On Sun, 4 Oct 2026 at 17:56, Zhijie Hou <houzhijie22@gmail.com> wrote:
In the worst case this could still lock a large number of tables, so the DDL
counts them and raises a clear error ("too many tables without a replica
identity in the publication") instead of a later "out of shared memory". This
is expected to be very rare, and since UPDATEs/DELETEs on those tables fail
anyway, the error should be acceptable because it hints that the publication
contains many tables that can't actually be published (since they lack RI).
I had considered this design as well, and it was my preferred approach
too. The main drawback is that a database or schema may contain a
large number of tables for which the user has not configured a replica
identity in the worst case as you pointed out. Locking all such tables
could consume a significant number of relation locks and potentially
exhaust the shared lock table before reaching the limit. If we are ok
with this trade-off, I think this design is reasonable.
Regards,
Vignesh
Hi,
On Mon, Oct 5, 2026 at 3:02 PM vignesh C <vignesh21@gmail.com> wrote:
On Sun, 4 Oct 2026 at 17:56, Zhijie Hou <houzhijie22@gmail.com> wrote:
In the worst case this could still lock a large number of tables, so the DDL
counts them and raises a clear error ("too many tables without a replica
identity in the publication") instead of a later "out of shared memory". This
is expected to be very rare, and since UPDATEs/DELETEs on those tables fail
anyway, the error should be acceptable because it hints that the publication
contains many tables that can't actually be published (since they lack RI).I had considered this design as well, and it was my preferred approach
too. The main drawback is that a database or schema may contain a
large number of tables for which the user has not configured a replica
identity in the worst case as you pointed out. Locking all such tables
could consume a significant number of relation locks and potentially
exhaust the shared lock table before reaching the limit. If we are ok
with this trade-off, I think this design is reasonable.
Right. I've seen reporters who are not comfortable with erroring out the DML,
and they'd prefer to error out the publication DDL if a table cannot be
published (lacking RI being one of the failure reasons), so I think this should
be acceptable.
Actually, I initially considered another, more invasive approach: completely
disallowing any ineligible table from being included in a published schema
(TABLES IN SCHEMA) or database (ALL TABLES). But I think the effect is too
broad — after writing a trivial patch to test it, many existing regression
tests started to fail. When you publish a schema or the whole database, there
are always some tables lacking a replica identity, and users just use ALL
TABLES for convenience since they're sure they'll only modify the valid tables,
while leaving the others unchanged or only performing inserts on them. So I'm
not sure this DDL error approach is great — maybe in the future we can
gradually move toward it, but I'm not sure it's OK to suddenly disallow all
these cases, especially for backbranches. The POC patch I shared is a kind of
hybrid approach: it starts reporting ERROR at DDL time, but only in the rare
case where too many tables are not publishable, so I personally feel it
could be more acceptable.
Best Regards,
Zhijie Hou
On Sun, Oct 4, 2026 at 5:56 PM Zhijie Hou <houzhijie22@gmail.com> wrote:
Hi,
On Tue, Sep 29, 2026 at 6:12 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 8 Sept 2026 at 10:54, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:I think this fix is not sufficient, as it does not address the ALTER PUBLICATION
SET (options) cases, where the publication action can also be altered
concurrently with DMLs, IIUC. The publication data in the relcache is also
affected by pubaction changes, so those should be blocked as well.Addressing the above should be sufficient for the row filter and column list
cases. However, for the replica identity check on UPDATE and DELETE operations,
further analysis may be needed - especially for the TABLES IN SCHEMA and ALL
TABLES cases, where tables are not explicitly published.One approach could be:
For "TABLES IN SCHEMA", lock the schema's namespace OID.
LockSchemaList() already takes a lock on the schema to prevent DROP
SCHEMA, so upgrade it from AccessShareLock to ShareRowExclusiveLock.
This keeps the existing protection and also prevents concurrent
writers from racing with CREATE PUBLICATION ... FOR TABLES IN SCHEMA
and ALTER PUBLICATION ... ADD/SET TABLES IN SCHEMA. For "FOR ALL
TABLES", lock the pg_publication relation with ShareRowExclusiveLock.
Take this lock in CreatePublication() when enabling FOR ALL TABLES,
and in AlterPublicationAllFlags() when changing puballtables from
false to true. On the DML side(UPDATE and DELETE),
CheckCmdReplicaIdentity() takes a matching RowExclusiveLock on the
table's namespace and pg_publication before using the publication
descriptor. This is needed only for tables without a local replica
identity; tables with a replica identity or REPLICA IDENTITY FULL are
not affected by this race.
RowExclusiveLock is self-compatible, so normal concurrent DML does not
block other DML. It conflicts with the ShareRowExclusiveLock taken by
the publication DDL, ensuring that the DDL and DML cannot race.
The attached POC demonstrates the changes for the same.Does this approach look reasonable, or is there a better way to handle
this synchronization?After considering more...
I think adding a heavy lock acquisition per DML statement sounds risky,
especially since this is a hot path that every table lacking a replica identity
will go through, whether or not it's published. Acquiring a lock even when no
conflict exists is not free either and the lock is held until transaction end.
I recall an argument [1] in another thread that adding an additional
heavyweight lock per transaction could limit the maximum achievable TPS. And
considering it's not rare to run pgbench with many backends doing UPDATE/DELETE
on tables lacking a replica identity, this needs more thoughts.
Rather than assuming there will be a regression with v2, I think it
would be good to test it first.
We could run pgbench with UPDATE/DELETE workloads on two types of
tables: one with a primary key (RI), and another with no RI. The table
with a RI should ideally see no impact. While the second case could
see some TPS regression, quantifying it would be useful. This data
will help us decide the next steps and approach to choose.
Show quoted text
I'm trying to explore whether we can narrow the scope to the DDL side as much
as possible, to avoid adding such a risky change to the hot path.Here is one idea, it's not perfect but I feel it's worth discussing.
The idea is to make TABLES IN SCHEMA or ALL TABLES publications lock the tables
only when they have neither a replica identity index nor replica identity FULL,
to prevent concurrent DML on these tables to execute. We only lock such tables,
because if we allow DML to write logs on these tables, the changes cannot be
replayed on the subscriber side due to lack of identity key. Nothing is locked
when the publication does not publish UPDATE or DELETE, since only those
actions require a replica identity. For tables named explicitly in a
publication, we increase the lock level as Vignesh's patch does, and
additionally lock their partition trees, the explicitly listed members when SET
(publish = ...) enables UPDATE or DELETE, and the tree of a dropped EXCEPT
exclusion.For new tables created concurrently, we can take a conflicting lock on the
publication catalog only when building the pubcache in
RelationBuildPublicationDesc(). The descriptor is built exactly when the
relcache entry is fresh or was invalidated, which means it's possible the table
was just created. The DDL holds the catalog lock until commit, so it cannot
commit between the descriptor build and the WAL write. This is the only lock
taken on the DML side, and steady-state DML never touches it. This also
protects the cases of tables that had a replica identity: if the identity index
is removed from the table, the cache will be rebuilt and the writer will take
the catalog lock, serializing with a concurrent publication DDL that may not
have seen the removal.In the worst case this could still lock a large number of tables, so the DDL
counts them and raises a clear error ("too many tables without a replica
identity in the publication") instead of a later "out of shared memory". This
is expected to be very rare, and since UPDATEs/DELETEs on those tables fail
anyway, the error should be acceptable because it hints that the publication
contains many tables that can't actually be published (since they lack RI).I'm sharing two POC patches for reference and discussion (they're not in great
shape and might have some bugs, but they basically implement the idea above).
0001 simply increases the lock level for explicitly included tables (FOR TABLE
a, b) in a publication, and additionally locks the child table when only the
parent is published. 0002 handles the TABLES IN SCHEMA and ALL TABLES cases,
locking tables lacking RI as described above.[1] /messages/by-id/CA+TgmoZzoLzcbrP9=Y3E=6xfyp=uH8CobowhuDV8fLbisQtbyA@mail.gmail.com
Best Regards,
Zhijie Hou
On v2, I see a deadlock because v2 locks the table first and then the
schema (a bottom-up approach). IIUC, this is the inverse order of
other commands, which take locks top-down (schema first, then table).
I think there could also be other such scenarios too that can
reproduce deadlock with v2. See this testcase:
CREATE SCHEMA test_schema;
CREATE TABLE test_schema.test_table (id int, val int);
INSERT INTO test_schema.test_table VALUES (1, 1);
Session A:
BEGIN;
--RowShareLock on table, just to make session B wait
SELECT * FROM test_schema.test_table FOR UPDATE;
Session B:
BEGIN;
-- Acquires AccessExclusiveLock on the schema.
-- Then blocks trying to lock the table, waiting on Session A.
DROP SCHEMA test_schema CASCADE;
Session A:
-- Upgrades table lock to RowExclusiveLock.
-- CheckCmdReplicaIdentity() then attempts to lock the schema.
UPDATE test_schema.test_table SET val = 2;
UPDATE fails with:
ERROR: deadlock detected
DETAIL: Process 20101 waits for RowExclusiveLock on object 16384 of
class 2615 of database 5; blocked by process 20115.
Process 20115 waits for AccessExclusiveLock on relation 16385 of
database 5; blocked by process 20101.
HINT: See server log for query details.
thanks
Shveta
Hi,
On Tue, Oct 6, 2026 at 5:23 PM shveta malik <shveta.malik@gmail.com> wrote:
On Sun, Oct 4, 2026 at 5:56 PM Zhijie Hou <houzhijie22@gmail.com> wrote:
After considering more...
I think adding a heavy lock acquisition per DML statement sounds risky,
especially since this is a hot path that every table lacking a replica identity
will go through, whether or not it's published. Acquiring a lock even when no
conflict exists is not free either and the lock is held until transaction end.
I recall an argument [1] in another thread that adding an additional
heavyweight lock per transaction could limit the maximum achievable TPS. And
considering it's not rare to run pgbench with many backends doing UPDATE/DELETE
on tables lacking a replica identity, this needs more thoughts.Rather than assuming there will be a regression with v2, I think it
would be good to test it first.We could run pgbench with UPDATE/DELETE workloads on two types of
tables: one with a primary key (RI), and another with no RI. The table
with a RI should ideally see no impact. While the second case could
see some TPS regression, quantifying it would be useful. This data
will help us decide the next steps and approach to choose.
I think the main point is that adding a new operation to a hot path like DML
requires a strong reason to convince ourselves and others. Even if the effect
is not visible in the test data, it's clear that acquiring a heavyweight lock
will add quite a few CPU cycles. Moreover, in the current simple DML case, only
3 locks are held till transaction end (target table, virtual XID, XID), and
we're going to add one more here, which seems questionable to me.
The root issue discussed here is that the relcache is stale due to insufficient
locking, and the standard approach to fixing such issues is to take a strong
lock during DDL rather than DML. The publication DDL case is special since it
needs to lock many tables, but I think we should build on that and improve
gradually rather than immediately strengthening the lock on the DML side. My
point is we should first put more effort into engineering the DDL side to solve
the issue, and if that turns out not to work, then we might consider the
DML-side approach as a last resort.
Of course, testing is also worthwhile, but I think even if the results show
nothing, we still need broad consensus if we want to choose that approach.
Best Regards,
Zhijie Hou
On Tue, 6 Oct 2026 at 17:50, Zhijie Hou <houzhijie22@gmail.com> wrote:
Hi,
On Tue, Oct 6, 2026 at 5:23 PM shveta malik <shveta.malik@gmail.com> wrote:
On Sun, Oct 4, 2026 at 5:56 PM Zhijie Hou <houzhijie22@gmail.com> wrote:
After considering more...
I think adding a heavy lock acquisition per DML statement sounds risky,
especially since this is a hot path that every table lacking a replica identity
will go through, whether or not it's published. Acquiring a lock even when no
conflict exists is not free either and the lock is held until transaction end.
I recall an argument [1] in another thread that adding an additional
heavyweight lock per transaction could limit the maximum achievable TPS. And
considering it's not rare to run pgbench with many backends doing UPDATE/DELETE
on tables lacking a replica identity, this needs more thoughts.Rather than assuming there will be a regression with v2, I think it
would be good to test it first.We could run pgbench with UPDATE/DELETE workloads on two types of
tables: one with a primary key (RI), and another with no RI. The table
with a RI should ideally see no impact. While the second case could
see some TPS regression, quantifying it would be useful. This data
will help us decide the next steps and approach to choose.I think the main point is that adding a new operation to a hot path like DML
requires a strong reason to convince ourselves and others. Even if the effect
is not visible in the test data, it's clear that acquiring a heavyweight lock
will add quite a few CPU cycles. Moreover, in the current simple DML case, only
3 locks are held till transaction end (target table, virtual XID, XID), and
we're going to add one more here, which seems questionable to me.The root issue discussed here is that the relcache is stale due to insufficient
locking, and the standard approach to fixing such issues is to take a strong
lock during DDL rather than DML. The publication DDL case is special since it
needs to lock many tables, but I think we should build on that and improve
gradually rather than immediately strengthening the lock on the DML side. My
point is we should first put more effort into engineering the DDL side to solve
the issue, and if that turns out not to work, then we might consider the
DML-side approach as a last resort.Of course, testing is also worthwhile, but I think even if the results show
nothing, we still need broad consensus if we want to choose that approach.
Thanks, let's proceed with this approach.
I found a few improvements that could be done:
1) Here I felt, we need not call find_all_inheritors for all
relations, we can call it only if the relation is
RELKIND_PARTITIONED_TABLE:
oldrel->relation = table_open(oldrelid,
- ShareUpdateExclusiveLock);
+ ShareRowExclusiveLock);
+ (void) find_all_inheritors(oldrelid, ShareRowExclusiveLock,
+ NULL);
2) Here there is a chance of overflow happening, better to use it like
how NLOCKENTS uses in lock.c:
foreach_oid(schemaid, schemas)
+ relids = list_concat(relids,
+ GetSchemaPublicationRelations(schemaid,
+
PUBLICATION_PART_LEAF));
+
+ maxlocks = (uint64) (max_locks_per_xact * (MaxBackends +
max_prepared_xacts - 1)) / 2;
3) This change is not required:
@@ -2033,6 +2066,7 @@ LockSchemaList(List *schemalist)
}
}
+
/*
The attached v3 version patch has the changes for the same.
Regards,
Vignesh
Attachments:
t253699_11v3-0001-Lock-tables-against-writers-when-altering-publica.patchapplication/octet-stream; name=v3-0001-Lock-tables-against-writers-when-altering-publica.patchDownload+233-14
v3-0002-Synchronize-publication-scope-widening-with.patchapplication/octet-stream; name=v3-0002-Synchronize-publication-scope-widening-with.patchDownload+465-7
Hi Vignesh,
The root issue discussed here is that the relcache is stale due to
insufficient
locking, and the standard approach to fixing such issues is to take a
strong
lock during DDL rather than DML. The publication DDL case is special
since it
needs to lock many tables, but I think we should build on that and
improve
gradually rather than immediately strengthening the lock on the DML
side. My
point is we should first put more effort into engineering the DDL side
to solve
the issue, and if that turns out not to work, then we might consider the
DML-side approach as a last resort.Of course, testing is also worthwhile, but I think even if the results
show
nothing, we still need broad consensus if we want to choose that
approach.
Thanks, let's proceed with this approach.
I found a few improvements that could be done:
Moving toward taking a stronger lock during DDL without penalizing DML
sounds attractive.
I tried v3 locally and found a lock-upgrade deadlock involving
pg_publication, even when the publications cover disjoint schemas.
CreatePublication() already holds RowExclusiveLock on pg_publication when
LockTablesWithoutReplicaIdentity() requests ShareLock on the same catalog.
Although two ShareLock requests are compatible, each conflicts with the
other transaction's existing RowExclusiveLock.
Here is a reproducer (the spec file is attached with this email as well):
-- Setup
CREATE SCHEMA review_a;
CREATE TABLE review_a.t (id int PRIMARY KEY, val int);
CREATE SCHEMA review_b;
CREATE TABLE review_b.t (id int PRIMARY KEY, val int);
CREATE TABLE review_outside (id int, val int);
INSERT INTO review_outside VALUES (1, 1);
-- Session 1
BEGIN;
UPDATE review_outside SET val = 2;
Although review_outside is not published, its descriptor build acquires
RowExclusiveLock on pg_publication because it lacks replica identity.
That lock is retained until transaction end.
-- Session 2
CREATE PUBLICATION review_pb FOR TABLES IN SCHEMA review_b;
Session 2 blocks requesting ShareLock on pg_publication while also holding
the RowExclusiveLock acquired by CreatePublication().
-- Session 1
CREATE PUBLICATION review_pa FOR TABLES IN SCHEMA review_a;
This deadlocks: Session 1 requests ShareLock while Session 2 holds
RowExclusiveLock, and Session 2 is waiting for Session 1's RowExclusiveLock.
The server reports both sessions waiting for ShareLock on relation
(pg_publication), each blocked by the other. The same sequence completes
without blocking on the unpatched build.
This cycle involves only the publication catalog locks, so sorting the
user-table locks does not address it.
A dedicated synchronization lock would avoid colliding with ordinary catalog
write locks and remove this additional lock-upgrade cycle. However, that would
not address the table/interlock deadlocks already acknowledged in the function
comment.
Regards,
Nikhil
---
Nikhil Sontakke
PlanetScale Postgres Core Team
Attachments:
On Fri, 9 Oct 2026 at 15:16, Nikhil Sontakke <nikhil@planetscale.com> wrote:
Moving toward taking a stronger lock during DDL without penalizing DML sounds attractive.
I tried v3 locally and found a lock-upgrade deadlock involving
pg_publication, even when the publications cover disjoint schemas.
Thanks Nikhil for reporting the issue, I could reproduce the issue
with the steps provided.
CreatePublication() already holds RowExclusiveLock on pg_publication when
LockTablesWithoutReplicaIdentity() requests ShareLock on the same catalog.
The synchronization protocol in LockTablesWithoutReplicaIdentity() /
RelationBuildPublicationDesc() was using the ordinary relation lock on
pg_publication via LockRelationOid(). Any concurrent CREATE/ALTER/DROP
PUBLICATION already holds a RowExclusiveLock on the same relation
while modifying the catalog. Since RowExclusiveLock is
self-compatible, these catalog locks do not block each other directly.
However, when each session subsequently requests a ShareLock, that
lock conflicts with the other session's still-held RowExclusiveLock,
creating a deadlock cycle.
Although two ShareLock requests are compatible, each conflicts with the
other transaction's existing RowExclusiveLock.Here is a reproducer (the spec file is attached with this email as well):
-- Setup
CREATE SCHEMA review_a;
CREATE TABLE review_a.t (id int PRIMARY KEY, val int);CREATE SCHEMA review_b;
CREATE TABLE review_b.t (id int PRIMARY KEY, val int);CREATE TABLE review_outside (id int, val int);
INSERT INTO review_outside VALUES (1, 1);-- Session 1
BEGIN;
UPDATE review_outside SET val = 2;Although review_outside is not published, its descriptor build acquires
RowExclusiveLock on pg_publication because it lacks replica identity.
That lock is retained until transaction end.-- Session 2
CREATE PUBLICATION review_pb FOR TABLES IN SCHEMA review_b;Session 2 blocks requesting ShareLock on pg_publication while also holding
the RowExclusiveLock acquired by CreatePublication().-- Session 1
CREATE PUBLICATION review_pa FOR TABLES IN SCHEMA review_a;This deadlocks: Session 1 requests ShareLock while Session 2 holds
RowExclusiveLock, and Session 2 is waiting for Session 1's RowExclusiveLock.The server reports both sessions waiting for ShareLock on relation
(pg_publication), each blocked by the other. The same sequence completes
without blocking on the unpatched build.This cycle involves only the publication catalog locks, so sorting the
user-table locks does not address it.A dedicated synchronization lock would avoid colliding with ordinary catalog
write locks and remove this additional lock-upgrade cycle.
Agreed, How about we use dedicated LockDatabaseObject() tag instead
of LockRelationOid():
/* LockTablesWithoutReplicaIdentity(), publicationcmds.c */
LockDatabaseObject(PublicationRelationId, InvalidOid, 0, ShareLock);
/* RelationBuildPublicationDesc(), relcache.c */
LockDatabaseObject(PublicationRelationId, InvalidOid, 0, RowExclusiveLock);
Using InvalidOid as the object ID is safe as a sentinel because it is
reserved and can never be a valid publication OID. Therefore, it
cannot conflict with AlterPublication()'s existing per-publication
lock on the same catalog.
However, that would
not address the table/interlock deadlocks already acknowledged in the function
comment.
Confirmed, and that is expected. The fix only removes the deadlock
cycle between operations that do not actually share any objects.
The attached top-up patch (on top of v3 patch shared at [1]) has the
changes for the same.
Thoughts?
Regards,
Vignesh
Attachments:
Fix_deadlock_issue.patchapplication/octet-stream; name=Fix_deadlock_issue.patchDownload+20-6
Hi Vignesh,
On Mon, Oct 12, 2026 at 3:07 PM vignesh C <vignesh21@gmail.com> wrote:
On Fri, 9 Oct 2026 at 15:16, Nikhil Sontakke <nikhil@planetscale.com>
wrote:Moving toward taking a stronger lock during DDL without penalizing DML
sounds attractive.
I tried v3 locally and found a lock-upgrade deadlock involving
pg_publication, even when the publications cover disjoint schemas.Thanks Nikhil for reporting the issue, I could reproduce the issue
with the steps provided.CreatePublication() already holds RowExclusiveLock on pg_publication when
LockTablesWithoutReplicaIdentity() requests ShareLock on the samecatalog.
The synchronization protocol in LockTablesWithoutReplicaIdentity() /
RelationBuildPublicationDesc() was using the ordinary relation lock on
pg_publication via LockRelationOid(). Any concurrent CREATE/ALTER/DROP
PUBLICATION already holds a RowExclusiveLock on the same relation
while modifying the catalog. Since RowExclusiveLock is
self-compatible, these catalog locks do not block each other directly.
However, when each session subsequently requests a ShareLock, that
lock conflicts with the other session's still-held RowExclusiveLock,
creating a deadlock cycle.Although two ShareLock requests are compatible, each conflicts with the
other transaction's existing RowExclusiveLock.Here is a reproducer (the spec file is attached with this email as well):
-- Setup
CREATE SCHEMA review_a;
CREATE TABLE review_a.t (id int PRIMARY KEY, val int);CREATE SCHEMA review_b;
CREATE TABLE review_b.t (id int PRIMARY KEY, val int);CREATE TABLE review_outside (id int, val int);
INSERT INTO review_outside VALUES (1, 1);-- Session 1
BEGIN;
UPDATE review_outside SET val = 2;Although review_outside is not published, its descriptor build acquires
RowExclusiveLock on pg_publication because it lacks replica identity.
That lock is retained until transaction end.-- Session 2
CREATE PUBLICATION review_pb FOR TABLES IN SCHEMA review_b;Session 2 blocks requesting ShareLock on pg_publication while also
holding
the RowExclusiveLock acquired by CreatePublication().
-- Session 1
CREATE PUBLICATION review_pa FOR TABLES IN SCHEMA review_a;This deadlocks: Session 1 requests ShareLock while Session 2 holds
RowExclusiveLock, and Session 2 is waiting for Session 1'sRowExclusiveLock.
The server reports both sessions waiting for ShareLock on relation
(pg_publication), each blocked by the other. The same sequence completes
without blocking on the unpatched build.This cycle involves only the publication catalog locks, so sorting the
user-table locks does not address it.A dedicated synchronization lock would avoid colliding with ordinary
catalog
write locks and remove this additional lock-upgrade cycle.
Agreed, How about we use dedicated LockDatabaseObject() tag instead
of LockRelationOid():
/* LockTablesWithoutReplicaIdentity(), publicationcmds.c */
LockDatabaseObject(PublicationRelationId, InvalidOid, 0, ShareLock);/* RelationBuildPublicationDesc(), relcache.c */
LockDatabaseObject(PublicationRelationId, InvalidOid, 0, RowExclusiveLock);Using InvalidOid as the object ID is safe as a sentinel because it is
reserved and can never be a valid publication OID. Therefore, it
cannot conflict with AlterPublication()'s existing per-publication
lock on the same catalog.However, that would
not address the table/interlock deadlocks already acknowledged in the
function
comment.
Confirmed, and that is expected. The fix only removes the deadlock
cycle between operations that do not actually share any objects.
The attached top-up patch (on top of v3 patch shared at [1]) has the
changes for the same.
Thoughts?
Thanks for the fix. I tested v3 with your top-up patch, and my original
reproducer now completes successfully.
However, a variation still produces a lock-upgrade deadlock with disjoint
publication scopes. Both transactions
first UPDATE separate, unpublished tables without replica identity, then
create publications for separate
schemas containing only tables with primary keys:
-- Setup
CREATE SCHEMA test_a;
CREATE TABLE test_a.t (id int PRIMARY KEY, val int);
CREATE SCHEMA test_b;
CREATE TABLE test_b.t (id int PRIMARY KEY, val int);
CREATE TABLE test_outside_a (id int, val int);
CREATE TABLE test_outside_b (id int, val int);
INSERT INTO test_outside_a VALUES (1, 1);
INSERT INTO test_outside_b VALUES (1, 1);
-- Session 1
BEGIN;
UPDATE test_outside_a SET val = 2;
-- Session 2
BEGIN;
UPDATE test_outside_b SET val = 2;
-- Session 1: blocks
CREATE PUBLICATION test_pa FOR TABLES IN SCHEMA test_a;
-- Session 2: deadlocks
CREATE PUBLICATION test_pb FOR TABLES IN SCHEMA test_b;
The same sequence completes without blocking on the unpatched build.
Looks like this needs more thinking. Could we explore making the dedicated
interlock follow the publication’s
scope? Descriptor builds for tables without replica identity could take
writer-side locks on both a schema-specific
tag and a database-wide tag. Publication DDL for TABLES IN SCHEMA would
take a conflicting lock only on the
relevant schema tags, while ALL TABLES would use the database-wide tag.
Explicit table publications would continue
using the stronger table locks. This could avoid conflicts between
unrelated schema scopes, although overlapping
scopes could still deadlock.
Regards,
Nikhil
---
Nikhil Sontakke
PlanetScale Postgres Core Team