On Mon, 28 Apr 2025 at 19:57, Álvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > On 2025-Apr-28, Shlok Kyal wrote: > > > 2. > > + * We also take a ShareLock on pg_partitioned_table to restrict addition > > + * of new partitioned table which may contain a foreign partition while > > + * publication is being created. XXX this is quite weird actually. > > > > This change was added to resolve the concurrency issue shared by > > Vignesh in [4]. I tried with different locks and found that lock with > > severity >= ShareLock was needed to avoid the concurrency issue. > > Initially I added ShareLock to pg_class, but to reduce the scope I > > added it to pg_partitioned_table instead. I cannot think of an > > alternate approach. Do you have any suggestions for this? > > > [4]: > > https://www.postgresql.org/message-id/CALDaNm2%2BeL22Sbvj74uS37xvt%3DhaQWcOwP15QnDuVeYsjHiffw%40mail.gmail.com > > I think this is the sticky point in this patch. I think you need a > clearer explanation (in code comments) of why you need this lock, and > whether a weaker lock would be enough in some cases (see below); also I > suspect that these locking considerations are going to be important for > users so they're going to need to be documented in the SGML docs. What > operations are blocked when you hold this lock? Is replication going to > block altogether until the transaction that runs > publication_check_foreign_parts() commits/aborts? This is important > because it might mean that that users need to keep such transactions > short.
ShareLock don't have any impact on replication.
For a partitioned table here is the observation when a ShareLock is
taken on the pg_partitioned table in a concurrent session:
1. CREATE TABLE (for partitioned table only) : blocked until
sharelock is released
2. INSERT : No impact
3 .UPDATE : No impact
4. TRUNCATE : No impact
5. DELETE : No impact
6. VACUUM : blocked until sharelock is released
7. DROP TABLE : blocked until sharelock is released
But I found one issue with the patch:
Suppose we have only a partitioned table t1 in a database. Now we
create a publication for all tables and add a breakpoint just after
the 'ShareLock'.
Now I run DROP TABLE t1; in a concurrent session. Now continue the
CREATE PUBLICATION command.
Now we hit a deadlock.
logs:
2025-05-09 13:35:57.199 IST [3567096] ERROR: deadlock detected
2025-05-09 13:35:57.199 IST [3567096] DETAIL: Process 3567096 waits
for AccessShareLock on relation 16411 of database 5; blocked by
process 3567751.
Process 3567751 waits for RowExclusiveLock on relation 3350 of
database 5; blocked by process 3567096.
Process 3567096: CREATE PUBLICATION pub1 FOR ALL TABLES WITH
(publish_via_partition_root);
Process 3567751: drop table t1;
This happens because the DROP TABLE command takes an
AccessExclusiveLock on the table t1 and is waiting to take
RowExclusiveLock on pg_partitioned_table. And
CREATE PUBLICATION command has taken ShareLock on pg_partitioned_table
and is waiting to take an AccessShareLock on table t1.
One thing I thought about is if we can change the ordering of the
locks while checking during creating publication, but it is not
possible as we must take lock on pg_partitoned_table first to avoid
creation of any partitioned table while we are fetching the table
list.
Do you have suggestion what should we do in above case?
> If your publication is FOR TABLES IN SCHEMA, can you do with blocking
> creation of partitions only in that schema, or only partitions of
> partitioned tables in that schema?
>
We should not block only a particular schema because we can create a
partition of a partitioned table in a different schema as well.
> Another point that just occurred to me is that pg_upgrade --check may
> need to alert users that if they have an incompatible setup in 18 or
> earlier, then an upgrade to 19 does not work until they have fixed the
> publications, detached the partitions, or some other remediation has
> been applied.
>
I have added a check in pg_upgrade and attached the same here.
Thanks and Regards,
Shlok Kyal
v14-0001-Restrict-publishing-of-partitioned-table-with-fo.patch
Description: Binary data
