Re: Restrict publishing of partitioned table with a foreign table as partition

On Mon, 28 Apr 2025 at 19:57, Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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