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

From: Shlok Kyal <[email-masked]>
Date: Mon, 28 Apr 2025 18:18:27 +0530

On Mon, 7 Apr 2025 at 18:09, Álvaro Herrera <[email-masked]> wrote:
>
> Here's the additional changes I made here before giving up on this.
> I think it needs some additional rethinking, not going to happen for 18.
>

Hi Alvaro,

Thanks for reviewing the patch.

The changes shared by you in [1], look good to me and I have added it
to the latest patch.
I have also included the changes shared by you in [2].
I have also addressed the issue reported by Sergey in [3].
Additionally, I made some changes to the comments, commit message and
documentation to reflect the changes in the function name and to
reduce the usage of 'foreign partition' term.

I saw some comments by you in the patches:
1.
+ /*
+ * Keep lock till end of transaction: must prevent this table from
+ * being attached a foreign table until we're done.  XXX does this
+ * prevent addition of a partition in a partitioned child?
+ */

This is same as the issue reported by Sergey in [3]. I have addressed
the issue in the latest patch. So, I have modified this comment.

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?

[1]: https://www.postgresql.org/message-id/202504062027.tqmabk2h353o%40alvherre.pgsql
[2]: https://www.postgresql.org/message-id/202504071239.kuj6m5a5wqxg%40alvherre.pgsql
[3]: https://www.postgresql.org/message-id/c64352fa-9a30-4e0a-853a-a6b5b6d07f4e%40postgrespro.ru
[4]: https://www.postgresql.org/message-id/CALDaNm2%2BeL22Sbvj74uS37xvt%3DhaQWcOwP15QnDuVeYsjHiffw%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 28 Apr 2025 18:21:46 +0530

On Mon, 7 Apr 2025 at 09:43, Sergey Tatarintsev
<[email-masked]> wrote:
>
> 07.04.2025 03:27, Álvaro Herrera пишет:
>
> On 2025-Apr-01, Shlok Kyal wrote:
>
> I have modified the comment in create_publication.sgml and also added
> comment in the restrictions section of logical-replication.sgml.
> I have also added a more detailed explanation in comment of
> 'check_foreign_tables'
>
> I have attached the updated v11 patch.
>
> Sadly I don't have time to describe the changes proposed here right now,
> but I'll do that early tomorrow.  (Some minor changes are still needed,
> particularly the comments to publication_check_foreign_parts which are
> mostly unchanged from what your patch has.  I'll do another review round
> tomorrow.)
>
> Hello!
>
> I looked at the latest patch again and found one more place for list_free(). Also look at additional test case:
>
> diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 51e463c112b..7fcc191feb9 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -442,6 +442,7 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,                           result = lappend_oid(result, partOid);                 } +               list_free(all_parts);         }         else                 result = lappend_oid(result, relid); diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 49c9d98b668..e56aebc397a 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -1296,6 +1296,14 @@ SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3'  -- foreign partition  ALTER PUBLICATION pub1 SET (publish_via_partition_root);   +CREATE SCHEMA sch5; +CREATE SCHEMA sch6; +CREATE TABLE sch6.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch5.part1 PARTITION OF sch6.tmain FOR VALUES FROM (0) TO (10) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch6.part2 PARTITION OF sch5.part1 FOR VALUES FROM (0) TO (5) SERVER fdw_server; +CREATE PUBLICATION pub4 FOR TABLES IN SCHEMA sch5 WITH (publish_via_partition_root); +SELECT pubname, tablename FROM pg_publication_tables WHERE pubname = 'pub4' ORDER BY pubname, tablename; +  DROP PUBLICATION pub1;  DROP PUBLICATION pub2;  DROP PUBLICATION pub3;
>
> I think this is a wrong assumption:
>
> ScanKeyInit(&key[keycount++], Anum_pg_class_relispartition, BTEqualStrategyNumber, F_BOOLEQ, BoolGetDatum(false));
>
> In this case sch5.part1 is partitioned table, but it also partition of table in different schema
>

Hi Sergey,

Thanks for reviewing the patch. I have addressed the comment in the
latest patch shared in [1].

[1]: https://www.postgresql.org/message-id/CANhcyEW0QMiJXMqpPFRHni-q0Rm4R0hpZ0LdaqA%3DF3wvDUU6sQ%40mail.gmail.com

Thanks and Regards,
Shlok Kyal

---

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

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

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 4 Jun 2025 20:41:59 +1000

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

Some quick comments on the patch:
1. In doc/src/sgml/ref/create_subscription.sgml:
+    has partitioned table with foreign table as partition. If this scenario is
+    detected we ERROR is logged to the user.
+   </para>
+

Should be: "If this scenario is detected an ERROR is logged to the
user." (remove "we").

In src/backend/commands/subscriptioncmds.c:
2. The comment header:
+ * This function is in charge of detecting if publisher with
+ * publish_via_partition_root=true publishes a partitioned table that has a
+ * foreign table as a partition.

Add "and throw an error if found" at the end of that sentence to
correctly describe what the function does.

3.
+    appendStringInfoString(&cmd,
+                           "SELECT DISTINCT P.pubname AS pubname "
+                           "from pg_catalog.pg_publication p, LATERAL "
+                           "pg_get_publication_tables(p.pubname) gpt, LATERAL "
+                           "pg_partition_tree(gpt.relid) gt JOIN
pg_catalog.pg_foreign_table ft ON "
+                           "ft.ftrelid = gt.relid WHERE p.pubviaroot
= true AND  p.pubname IN (");

use FROM rather than from to maintain SQL style consistency.

4.
+                errdetail_plural("The subscription being created on a
publication (%s) with publish_via_root_partition = true and contains
partitioned tables with foreign table as partition ",
+                                 "The subscription being created on
publications (%s) with publish_via_root_partition = true and contains
partitioned tables with foreign table as partition ",
+                                 list_length(publist), pubnames->data),

I think you meant "publish_via_partition_root" here and not
"publish_via_root_partition ".

regards,
Ajin Cherian
Fujitsu Australia

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 9 Jun 2025 11:27:55 +0530

> "publish_via_root_partition ".

I have addressed all the comments and attached the updated patch.

Thanks and Regards,
Shlok Kyal

---

From: Ajin Cherian <[email-masked]>
Date: Tue, 1 Jul 2025 12:50:41 +1000

> I have addressed all the comments and attached the updated patch.

Hi Shlok,

Some more comments:
1.
+     Replication is not supported for foreign tables.  When used as partitions
+     of partitioned tables, publishing of the partitioned table is only allowed
+     if the <literal>publish_via_partition_root</literal> is set to
+     <literal>false</literal>.

In patch: "When used as partitions of partitioned tables, publishing
of the partitioned table is only allowed if the
<literal>publish_via_partition_root</literal> is set to
<literal>false</literal>.

Change to: When foreign tables are used as partitions of partitioned
tables, publishing of the partitioned table is only allowed if the
<literal>publish_via_partition_root</literal> is set to
<literal>false</literal>.

2.
+   <para>
+    When using a subscription parameter copy_data = true, corresponding
+    publications are checked if it has publish_via_partition_root = true and
+    has partitioned table with foreign table as partition. If this scenario is
+    detected an ERROR is logged to the user.
+   </para>

In patch: "When using a subscription parameter copy_data = true, ..."
Change to: "When using the subscription parameter copy_data = true, ..."

In patch: "and has partitioned table with foreign table as partition."
Change to: "and has a partitioned table with a foreign table as its partition"

3.
+ * check_publications_foreign_parts
+ * Check if the publications, on which subscriber is subscribing, publishes any
+ * partitioned table that has an foreign table as its partition and has
+ * publish_via_partition_root set as true. The check is performed only if
+ * copy_data is set as true for the subscription.

In patch: "publishes any partitioned table that has an foreign table
as its partition"
Change to: "publishes any partitioned table that has a foreign table
as its partition"

4.
+ * DML data changes are not published for data in foreign tables,
+ * and yet the tablesync worker is not smart enough to omit data from
+ * foreign tables when they are partitions of partitioned tables.

change to:"Although DML changes to foreign tables are excluded from
publication, the tablesync worker will still attempt to copy data from
foreign table partitions during initial table synchronization."

5.
+ * When publish_via_partition_root is false, each partition published for
+ * replication is listed individually in pg_subscription_rel, and we
+ * don't add partitions that are foreign tables, so this check is not
+ * needed.

In patch: "so this check is not needed"
Change to: "so this function is not called for such tables"

6.
+                errdetail_plural("The subscription being created on a
publication (%s) with publish_via_partition_root = true and contains
partitioned tables with foreign table as partition ",
+                                 "The subscription being created on
publications (%s) with publish_via_partition_root = true and contains
partitioned tables with foreign table as partition ",
+                                 list_length(publist), pubnames->data),

Change to: "The subscription is for a publication (%s) with
publish_via_partition_root = true but one of the partitioned tables
has a foreign table as a partition"
"The subscription is for publications (%s) with
publish_via_partition_root = true but one of the partitioned tables
has a foreign table as a partition"

7.
+        /* capture all publications that include this relation directly */
+        puboids = list_concat(puboids, GetRelationPublications(rel->rd_id));
+        schemaid = RelationGetNamespace(rel);
+        puboids = list_concat(puboids, GetSchemaPublications(schemaid));
+
+        /* and do the same for its ancestors, if any */
+        ancestors = get_partition_ancestors(rel->rd_id);
+        foreach_oid(ancestor, ancestors)
+        {
+            puboids = list_concat(puboids, GetRelationPublications(ancestor));
+            schemaid = get_rel_namespace(ancestor);
+            puboids = list_concat(puboids, GetSchemaPublications(schemaid));
+        }
+
+        /* Now check the publish_via_partition_root bit for each of those */
+        list_sort(puboids, list_oid_cmp);
+        list_deduplicate_oid(puboids);
+        foreach_oid(puboid, puboids)

Why do we need to do all this logic for non partition foreign tables?
Just directly throw an error for those tables and do this extra logic
only for partitioned tables.

8.
+        pg_fatal("Your installation contains publications, which has
foreign table which are partitions of published table.\n"
+                 "A list of potentially-affected publications is in
the file:\n"

Change to: "Your installation contains publications, where one of the
partitioned tables has a foreign table as a partition.\n"

regards,
Ajin Cherian
Fujitsu Australia

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 4 Jul 2025 10:33:34 +0530

> <literal>false</literal>.
Fixed

> Change to: "and has a partitioned table with a foreign table as its partition"
Fixed

> Change to: "publishes any partitioned table that has a foreign table
Fixed

> foreign table partitions during initial table synchronization."
Fixed

> Change to: "so this function is not called for such tables"
Fixed

> has a foreign table as a partition"
"one of the partitioned tables" this would be misleading as
publication can have multiple partitioned table which have foreign
table as its partition.
I have reworded it like:
"The subscription is for a publication (%s) with
publish_via_partition_root = true, but one or more partitioned tables
have foreign tables as partitions.",
"The subscription is for publications (%s) with
publish_via_partition_root = true but one or more partitioned tables
have foreign tables as partitions."

> only for partitioned tables.
We need to throw an error if foreign table is being attached to a
partitioned table which is already published (or its parent is already
published) with publish_via_partition_root is true.
So, this logic is required to check if the table to which it is being
attached is published with  publish_via_partition_root or not.

> partitioned tables has a foreign table as a partition.\n"
There can be mutiple partitioned table published. Reworded the error as:
"Your installation contains publications where one or more partitioned
tables have foreign tables as partitions.\n"

Thanks for reviewing the patch. I have addressed the comments and
attached the latest patch.

Thanks and Regards,
Shlok Kyal