Feedback-ID: ia2694551:Fastmail
Date: Mon, 7 Apr 2025 14:39:23 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, 
	Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>, pgsql-hackers@lists.postgresql.org
Subject: Re: Restrict publishing of partitioned table with a foreign table as
 partition
Message-ID: <202504071239.kuj6m5a5wqxg@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="4teue7korxzhxvy2"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202504062027.tqmabk2h353o@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/202504071239.kuj6m5a5wqxg%40alvherre.pgsql>
Precedence: bulk


--4teue7korxzhxvy2
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

--4teue7korxzhxvy2
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment; filename="0001-some-more-changes.patch.txt"

From cc7b4021d5661aa8dbb8bd3a8c8569824dbbbc36 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Mon, 7 Apr 2025 14:31:44 +0200
Subject: [PATCH] some more changes

---
 src/backend/catalog/pg_publication.c      | 63 ++++++++++++-----------
 src/test/regress/expected/publication.out | 16 ++++++
 src/test/regress/sql/publication.sql      | 10 ++++
 3 files changed, 59 insertions(+), 30 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 51e463c112b..ec76c0c8eb2 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -34,6 +34,7 @@
 #include "commands/publicationcmds.h"
 #include "funcapi.h"
 #include "partitioning/partdesc.h"
+#include "storage/lmgr.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/catcache.h"
@@ -130,53 +131,56 @@ check_publication_add_schema(Oid schemaid)
 
 /*
  * publication_check_foreign_parts
- *		Helper function to ensure we don't publish foreign tables
+ *		Helper function to ensure we don't indirectly publish foreign tables
  *
- * Foreign tables may not be published.
+ * 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.  To
+ * avoid the inconsistencies that would result, we disallow foreign
+ * tables from being published generally.  However, it's possible for
+ * partitioned tables to have foreign tables as partitions, and we would
+ * like to allow publishing those partitioned tables so that the other
+ * partitions are replicated.
  *
- * Protect against including foreign tables that are partitions of partitioned
- * tables published by the given publication when publish_via_root_partition is
- * true. This will not work correctly as the initial data from the foreign
- * table can be replicated by the tablesync worker even though replication of
- * foreign table is not supported because when publish_via_root_partition is
- * true, the root table is included in pg_subscription_rel catalog table and
- * tablesync worker cannot distinguish data from foreign partition. So we
- * disallow the case here and in all DDL commands that would end up creating
- * such a case indirectly.
+ * This function is in charge of detecting if a partitioned table has a
+ * foreign table as a partition -- either in the whole database (useful
+ * for FOR ALL TABLES publications) or in a particular schema (useful
+ * for FOR TABLES IN SCHEMA publications).  This function must be called
+ * only for publications with publish_via_partition_root=true.
  *
- * When publish_via_root_partition is set to false, leaf partitions are included
- * in pg_subscription_rel catalog table. So, when we include a partition table
- * with foreign partition in a publication, we skip including foreign partitions
- * to pg_subscription_rel catalog table. So, the foreign partitions are not
- * replicated.
+ * When publish_via_partition_root is false, each partition published for
+ * replication is listed individually in pg_replication_rel, and we
+ * don't add partitions that are foreign tables, so this check is not
+ * needed.
  *
+ * If a valid schemaid is provided, check if that schema has any
+ * partitioned table with a foreign table as partition.
  *
+ * If no valid schemaid is provided, check all partitioned tables.
  *
- * If valid schemaid provided, check if the schema has a partition table which
- * has a foreign partition. The partition tables in a schema can have partitions
- * in other schema. We also need to check if such partitions are foreign
- * partition.
+ * We take a lock on partition tables so no new foreign partitions are
+ * added concurrently.
  *
- * If valid schemaid is not provided, we get all partition tables and check if
- * it has any foreign partition. We take a lock on partition tables so no new
- * foreign partitions are added concurrently.
- *
- * We 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.
+ * 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.
  */
 void
 publication_check_foreign_parts(Oid schemaid, char *pubname)
 {
 	Relation	classRel;
-	Relation	partRel;
 	ScanKeyData key[3];
 	int			keycount = 0;
 	TableScanDesc scan;
 	HeapTuple	tuple;
 
+	/*
+	 * Take lock on pg_partitioned_rel.  This prevents new publications
+	 * from being created.
+	 */
+	LockRelationOid(PartitionedRelationId, ShareLock);
+
 	classRel = table_open(RelationRelationId, AccessShareLock);
-	partRel = table_open(PartitionedRelationId, ShareLock);
 
 	/* Get the root nodes of partitioned table */
 	ScanKeyInit(&key[keycount++],
@@ -222,7 +226,6 @@ publication_check_foreign_parts(Oid schemaid, char *pubname)
 
 	table_endscan(scan);
 	table_close(classRel, AccessShareLock);
-	table_close(partRel, NoLock);
 }
 
 /*
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 4395563fcd5..678f8b52e5b 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -2006,6 +2006,22 @@ SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3'
 ALTER PUBLICATION pub1 SET (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.
+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);	-- should fail
+ERROR:  cannot add table ...
+SELECT pubname, tablename FROM pg_publication_tables WHERE pubname = 'pub4' ORDER BY pubname, tablename;
+ pubname | tablename 
+---------+-----------
+ pub4    | part1
+(1 row)
+
+DROP SCHEMA sch5, sch6 CASCADE;
+DROP PUBLICATION pub4;
 DROP PUBLICATION pub1;
 DROP PUBLICATION pub2;
 DROP PUBLICATION pub3;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 49c9d98b668..43755b973cb 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1296,6 +1296,16 @@ 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 SCHEMA sch5, sch6 CASCADE;
+DROP PUBLICATION pub4;
+
 DROP PUBLICATION pub1;
 DROP PUBLICATION pub2;
 DROP PUBLICATION pub3;
-- 
2.39.5


--4teue7korxzhxvy2--