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

From: Álvaro Herrera <[email-masked]>
Date: Mon, 28 Apr 2025 16:27:20 +0200

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.

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?

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.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Linux transformó mi computadora, de una `máquina para hacer cosas',
en un aparato realmente entretenido, sobre el cual cada día aprendo
algo nuevo" (Jaime Salinas)

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 9 May 2025 20:25:17 +0530

> because it might mean that that users need to keep such transactions

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?

> 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.

> publications, detached the partitions, or some other remediation has
I have added a check in pg_upgrade and attached the same here.

Thanks and Regards,
Shlok Kyal

---

From: Álvaro Herrera <[email-masked]>
Date: Sat, 10 May 2025 13:59:27 -0400

Hello,

I think reimplementing list_member_oid() under a different name
(is_ancestor_member_relids) is pointless and should not be done.

It also appears to me that we haven't nailed the error messages just
yet.  I tried to fix it upthread, but didn't really get it correct.  For
instance, consider this case taken from the new regression tests:

/* test setup */
CREATE SCHEMA sch3;
CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id);
CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5);
CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id);
CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server;
ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10);

The issue occurs in the next command:

 CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root);
 ERROR:  cannot add partitioned table "tmain" to publication "pub1", which has "publish_via_partition_root" set to "true"
 DETAIL:  The table contains a partition that's a foreign table.

The user is trying to create a publication, so an error saying that it's
not possible to add a partition is a bit off-point.  (If you squint hard
enough it sort of makes sense, but it seems inconsistent enough.)
We could replace the word "add" with the word "include" there and this
particular problem would be fixed, perhaps.  However, the next one is
worse:

  CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root);
  ERROR:  cannot set parameter "publish_via_partition_root" to "true" for publication "pub1"
  DETAIL:  Published partitioned table "tmain" contains a partition that is a foreign table.

Here we again try to create a publication, but we get an error that we
cannot set the parameter, which is even more detached from reality.
Maybe what we need is to add another parameter to that function to
indicate which operation is being executed (create publication, add
schema, change the parameter), which is used to indicate the error
message to throw.

With that thought in mind, I think it would make things simpler for us
to remove the duplicate copies of the same error messages by adding yet
another function, after publication_check_foreign_parts(), which receives
the operation being executed and whatever additional parameter it needs,
and throws the error.  If publication_check_foreign_parts() also
receives the operation being executed, it can simply pass it through to
that new function when an error needs to be thrown; other places that
hardcode error situations would pass a constant for the operation.

But the non-idiomatic locking of pg_partitioned_table appears to
continue to be the pain point of this patch.  My impression is that
using a lock is the wrong approach to solve the concurrency problem.
Maybe we can use a ConditionVariable instead somehow.  (The real trick
here is to figure out exactly _how_ to use the CV, I mean what exactly
is the condition that the CV sleeps on?)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees."                                      (E. Dijkstra)

---

From: Amit Kapila <[email-masked]>
Date: Thu, 15 May 2025 18:18:58 +0530

> is the condition that the CV sleeps on?)

Can we fix this problem by having a check at the time of
CREATESUBSCRIPTION such that, if copy-data is true, then we ensure
that the specified publishers don't have a foreign table? We have a
somewhat similar check for publications in the function
check_publications_origin(), though for a different purpose. Along
with it, we can still have a check during foreign table
creation/attach that it doesn't become part of some publication, as
the patch may have it now.

-- 
With Regards,
Amit Kapila.

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 19 May 2025 22:02:41 +0530

> the patch may have it now.

This approach seems better to me. I have created a patch with the
above approach.

Thanks and Regards,
Shlok Kyal