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

From: Álvaro Herrera <[email-masked]>
Date: Sun, 6 Apr 2025 22:27:05 +0200

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

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
>From c6433548f3f47b23b81384f3cc80c0df307f6415 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email-masked]>
Date: Sun, 6 Apr 2025 21:27:39 +0200
Subject: [PATCH] some changes

---
 doc/src/sgml/logical-replication.sgml     |  18 +-
 doc/src/sgml/ref/create_publication.sgml  |  24 ++-
 src/backend/catalog/pg_publication.c      | 235 ++++++++++------------
 src/backend/commands/foreigncmds.c        |  49 -----
 src/backend/commands/publicationcmds.c    |  42 ++--
 src/backend/commands/tablecmds.c          | 109 +++++++---
 src/backend/partitioning/partdesc.c       |  32 +++
 src/include/catalog/pg_publication.h      |   6 +-
 src/include/partitioning/partdesc.h       |   1 +
 src/test/regress/expected/publication.out |  30 +--
 10 files changed, 287 insertions(+), 259 deletions(-)

diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index ce60a1b391c..67503aec871 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -2154,18 +2154,18 @@ CONTEXT:  processing remote data for replication origin "pg_16395" during "INSER
 
    <listitem>
     <para>
-     Replication is supported only for tables, including partitioned tables,
-     except when they contain foreign partitions. If
-     <literal>publish_via_partition_root</literal> is set to
-     <literal>true</literal> for a publication table with foreign partitions, or
-     if an attempt is made to replicate such a table, an error is thrown.
-     Additionally, when replicating a partitioned table where
-     <literal>publish_via_partition_root</literal> is set to
-     <literal>false</literal> and foreign partitions are present, all partitions
-     are replicated except the foreign partitions.
+     Replication is only supported for tables, including partitioned tables.
      Attempts to replicate other types of relations, such as views, materialized
      views, or foreign tables, will result in an error.
     </para>
+    <para>
+     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 this mode, changes to foreign partitions are
+     ignored for the purposes of replication, and data contained in them
+     is not included during initial synchronization.
+    </para>
    </listitem>
 
    <listitem>
diff --git a/doc/src/sgml/ref/create_publication.sgml b/doc/src/sgml/ref/create_publication.sgml
index 47216fc4789..9f412e6693a 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -248,20 +248,26 @@ CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
           that of the individual partitions.
          </para>
 
+         <para>
+          If this parameter is enabled, <literal>TRUNCATE</literal>
+          operations performed directly on partitions are not replicated.
+         </para>
+
+         <para>
+          If this parameter is enabled, foreign tables and partitioned tables
+          containing partitions that are foreign tables may not be
+          added to the publication.  Conversely, foreign tables may not be
+          attached to a partitioned table that is included in a publication
+          with this parameter enabled.  Lastly, this parameter may not be
+          changed on publications that include partitioned tables with foreign
+          tables as partitions.
+         </para>
+
          <para>
           This parameter also affects how row filters and column lists are
           chosen for partitions; see below for details.
          </para>
 
-         <para>
-          If this is enabled, <literal>TRUNCATE</literal> operations performed
-          directly on partitions are not replicated.
-         </para>
-
-         <para>
-          If this is enabled, a foreign table or a partitioned table with a
-          foreign partition is not allowed in the publication.
-         </para>
         </listitem>
        </varlistentry>
       </variablelist></para>
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 77299b75cde..51e463c112b 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -55,7 +55,7 @@ typedef struct
  * error if not.
  */
 static void
-check_publication_add_relation(Relation targetrel, Publication *pub)
+check_publication_add_relation(Publication *pub, Relation targetrel)
 {
 	/* Must be a regular or partitioned table */
 	if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION &&
@@ -68,15 +68,18 @@ check_publication_add_relation(Relation targetrel, Publication *pub)
 
 	/*
 	 * publish_via_root_partition cannot be true if it is a partitioned table
-	 * and has any foreign partition. See check_foreign_tables for details.
+	 * and has any foreign partition. See publication_check_foreign_parts for
+	 * details.
 	 */
-	if (pub->pubviaroot && check_partrel_has_foreign_table(targetrel))
+	if (pub->pubviaroot &&
+		targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+		RelationHasForeignPartition(targetrel))
 		ereport(ERROR,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("cannot set parameter \"%s\" to true for publication \"%s\"",
-						"publish_via_partition_root", pub->name),
-				 errdetail("partition table \"%s\" in publication contains a foreign partition",
-						   RelationGetRelationName(targetrel))));
+				errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				errmsg("cannot add partitioned table \"%s\" to publication \"%s\", which has \"%s\" set to \"%s\"",
+					   RelationGetRelationName(targetrel), pub->name,
+					   "publish_via_partition_root", "true"),
+				errdetail("The table contains a partition that's a foreign table."));
 
 	/* Can't be system table */
 	if (IsCatalogRelation(targetrel))
@@ -125,6 +128,103 @@ check_publication_add_schema(Oid schemaid)
 				 errdetail("Temporary schemas cannot be replicated.")));
 }
 
+/*
+ * publication_check_foreign_parts
+ *		Helper function to ensure we don't publish foreign tables
+ *
+ * Foreign tables may not be published.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ *
+ *
+ * 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.
+ *
+ * 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.
+ */
+void
+publication_check_foreign_parts(Oid schemaid, char *pubname)
+{
+	Relation	classRel;
+	Relation	partRel;
+	ScanKeyData key[3];
+	int			keycount = 0;
+	TableScanDesc scan;
+	HeapTuple	tuple;
+
+	classRel = table_open(RelationRelationId, AccessShareLock);
+	partRel = table_open(PartitionedRelationId, ShareLock);
+
+	/* Get the root nodes of partitioned table */
+	ScanKeyInit(&key[keycount++],
+				Anum_pg_class_relkind,
+				BTEqualStrategyNumber, F_CHAREQ,
+				CharGetDatum(RELKIND_PARTITIONED_TABLE));
+
+	ScanKeyInit(&key[keycount++],
+				Anum_pg_class_relispartition,
+				BTEqualStrategyNumber, F_BOOLEQ,
+				BoolGetDatum(false));
+
+	/* If schema id is provided check partitioned table in that schema */
+	if (OidIsValid(schemaid))
+		ScanKeyInit(&key[keycount++],
+					Anum_pg_class_relnamespace,
+					BTEqualStrategyNumber, F_OIDEQ,
+					schemaid);
+
+	scan = table_beginscan_catalog(classRel, keycount, key);
+	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+	{
+		Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+		Relation	pubrel = table_open(relForm->oid, AccessShareLock);
+
+		if (pubrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+			RelationHasForeignPartition(pubrel))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("cannot set parameter \"%s\" to \"%s\" for publication \"%s\"",
+							"publish_via_partition_root", "true", pubname),
+					 errtable(pubrel),
+					 errdetail("Published partitioned table \"%s\" contains a partition that is a foreign table.",
+							   get_rel_name(relForm->oid))));
+
+		/*
+		 * 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?
+		 */
+		table_close(pubrel, NoLock);
+	}
+
+	table_endscan(scan);
+	table_close(classRel, AccessShareLock);
+	table_close(partRel, NoLock);
+}
+
 /*
  * Returns if relation represented by oid and Form_pg_class entry
  * is publishable.
@@ -473,7 +573,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
 						RelationGetRelationName(targetrel), pub->name)));
 	}
 
-	check_publication_add_relation(targetrel, pub);
+	check_publication_add_relation(pub, targetrel);
 
 	/* Validate and translate column names into a Bitmapset of attnums. */
 	attnums = pub_collist_validate(pri->relation, pri->columns);
@@ -718,7 +818,7 @@ publication_add_schema(Oid pubid, Oid schemaid, bool if_not_exists)
 	 * partition
 	 */
 	if (pub->pubviaroot)
-		check_foreign_tables(schemaid, pub->name);
+		publication_check_foreign_parts(schemaid, pub->name);
 
 	/* Form a tuple */
 	memset(values, 0, sizeof(values));
@@ -1349,118 +1449,3 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
 
 	SRF_RETURN_DONE(funcctx);
 }
-
-/* Check if a partitioned table has a foreign partition */
-bool
-check_partrel_has_foreign_table(Relation rel)
-{
-	bool		has_foreign_tbl = false;
-
-	if (RelationGetForm(rel)->relkind == RELKIND_PARTITIONED_TABLE)
-	{
-		PartitionDesc pd = RelationGetPartitionDesc(rel, true);
-
-		for (int i = 0; i < pd->nparts; i++)
-		{
-			Relation	childrel = table_open(pd->oids[i], AccessShareLock);
-
-			if (RelationGetForm(childrel)->relkind == RELKIND_FOREIGN_TABLE)
-				has_foreign_tbl = true;
-			else
-				has_foreign_tbl = check_partrel_has_foreign_table(childrel);
-
-			table_close(childrel, NoLock);
-
-			if (has_foreign_tbl)
-				break;
-		}
-	}
-
-	return has_foreign_tbl;
-}
-
-/*
- * 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.
- *
- * 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.
- *
- *
- * check_foreign_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.
- *
- * 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.
- */
-void
-check_foreign_tables(Oid schemaid, char *pubname)
-{
-	Relation	classRel;
-	Relation	partRel;
-	ScanKeyData key[3];
-	int			keycount = 0;
-	TableScanDesc scan;
-	HeapTuple	tuple;
-
-	classRel = table_open(RelationRelationId, AccessShareLock);
-	partRel = table_open(PartitionedRelationId, ShareLock);
-
-	/* Get the root nodes of partitioned table */
-	ScanKeyInit(&key[keycount++],
-				Anum_pg_class_relkind,
-				BTEqualStrategyNumber, F_CHAREQ,
-				CharGetDatum(RELKIND_PARTITIONED_TABLE));
-
-	ScanKeyInit(&key[keycount++],
-				Anum_pg_class_relispartition,
-				BTEqualStrategyNumber, F_BOOLEQ,
-				BoolGetDatum(false));
-
-	/* If schema id is provided check partitioned table in that schema */
-	if (OidIsValid(schemaid))
-		ScanKeyInit(&key[keycount++],
-					Anum_pg_class_relnamespace,
-					BTEqualStrategyNumber, F_OIDEQ,
-					schemaid);
-
-	scan = table_beginscan_catalog(classRel, keycount, key);
-	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
-	{
-		Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
-		Relation	pubrel = table_open(relForm->oid, AccessShareLock);
-
-		if (check_partrel_has_foreign_table(pubrel))
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("cannot set parameter \"%s\" to true for publication \"%s\"",
-							"publish_via_partition_root", pubname),
-					 errdetail("partition table \"%s\" in publication contains a foreign partition",
-							   get_rel_name(relForm->oid))));
-
-		table_close(pubrel, NoLock);
-	}
-
-	table_endscan(scan);
-	table_close(classRel, AccessShareLock);
-	table_close(partRel, NoLock);
-}
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index 98c1c82db61..c6843a9c30a 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -1424,55 +1424,6 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid)
 
 	ftrel = table_open(ForeignTableRelationId, RowExclusiveLock);
 
-	/*
-	 * Check if it is a foreign partition and the partitioned table is not
-	 * published or published with publish_via_partition_root option as false.
-	 * See check_foreign_tables for details.
-	 */
-	if (stmt->base.partbound != NULL)
-	{
-		RangeVar   *root = linitial_node(RangeVar, stmt->base.inhRelations);
-		Relation	rootrel = table_openrv(root, AccessShareLock);
-
-		if (RelationGetForm(rootrel)->relkind == RELKIND_PARTITIONED_TABLE)
-		{
-			Oid			schemaid = RelationGetNamespace(rootrel);
-			List	   *puboids = GetRelationPublications(rootrel->rd_id);
-			List	   *ancestors;
-
-			puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
-			puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid));
-			ancestors = get_partition_ancestors(rootrel->rd_id);
-
-			foreach_oid(ancestor, ancestors)
-			{
-				puboids = list_concat_unique_oid(puboids,
-												 GetRelationPublications(ancestor));
-				schemaid = get_rel_namespace(ancestor);
-				puboids = list_concat_unique_oid(puboids,
-												 GetSchemaPublications(schemaid));
-			}
-			list_free(ancestors);
-
-			foreach_oid(puboid, puboids)
-			{
-				Publication *pub = GetPublication(puboid);
-
-				if (pub->pubviaroot)
-					ereport(ERROR,
-							(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-							 errmsg("cannot create table foreign partition \"%s\"",
-									get_rel_name(relid)),
-							 errdetail("partition table \"%s\" is published with option publish_via_partition_root",
-									   RelationGetRelationName(rootrel))));
-			}
-
-			list_free(puboids);
-		}
-
-		table_close(rootrel, AccessShareLock);
-	}
-
 	/*
 	 * For now the owner cannot be specified on create. Use effective user ID.
 	 */
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index b06960e8e22..54f2b505e3b 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -35,6 +35,7 @@
 #include "commands/publicationcmds.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
+#include "partitioning/partdesc.h"
 #include "parser/parse_clause.h"
 #include "parser/parse_collate.h"
 #include "parser/parse_relation.h"
@@ -917,7 +918,7 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
 	{
 		/* Check if any partitioned table has foreign partition */
 		if (publish_via_partition_root)
-			check_foreign_tables(InvalidOid, stmt->pubname);
+			publication_check_foreign_parts(InvalidOid, stmt->pubname);
 
 		/* Invalidate relcache so that publication info is rebuilt. */
 		CacheInvalidateRelcacheAll();
@@ -1086,45 +1087,42 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt,
 
 	/*
 	 * If publish_via_partition_root is set to true, check if the publication
-	 * already have any foreign partition. See check_foreign_tables for details.
+	 * has any foreign partition.  See publication_check_foreign_parts for
+	 * details.
 	 */
 	if (publish_via_partition_root_given && publish_via_partition_root)
 	{
-		List	   *schemaoids = NIL;
-		List	   *relids = NIL;
-
 		char	   *pubname = stmt->pubname;
+		List	   *schemaoids;
+		List	   *relids;
 
 		if (pubform->puballtables)
-			check_foreign_tables(InvalidOid, pubname);
+			publication_check_foreign_parts(InvalidOid, pubname);
 
 		schemaoids = GetPublicationSchemas(pubform->oid);
-
 		foreach_oid(schemaoid, schemaoids)
-			check_foreign_tables(schemaoid, pubname);
-
-		list_free(schemaoids);
+			publication_check_foreign_parts(schemaoid, pubname);
 
 		relids = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
-
 		foreach_oid(relid, relids)
 		{
-			Relation	pubrel = table_open(relid, AccessShareLock);
+			Relation	pubrel;
 
-			if (check_partrel_has_foreign_table(pubrel))
-			{
+			if (get_rel_relkind(relid) != RELKIND_PARTITIONED_TABLE)
+				continue;
+
+			pubrel = table_open(relid, AccessShareLock);
+
+			if (RelationHasForeignPartition(pubrel))
 				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("cannot set parameter \"%s\" to true for publication \"%s\"",
-								"publish_via_partition_root", pubname),
-						 errdetail("partition table \"%s\" in publication contains a foreign partition",
-								   get_rel_name(relid))));
-			}
+						errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						errmsg("cannot set parameter \"%s\" to \"%s\" for publication \"%s\"",
+							   "publish_via_partition_root", "true", pubname),
+						errdetail("Published partitioned table \"%s\" contains a partition that is a foreign table.",
+								  RelationGetRelationName(pubrel)));
 
 			table_close(pubrel, NoLock);
 		}
-
-		list_free(relids);
 	}
 
 	/* Everything ok, form a new tuple. */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eed4487e2d9..b594b01e774 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1129,6 +1129,53 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
 					 errmsg("\"%s\" is not partitioned",
 							RelationGetRelationName(parent))));
 
+		/*
+		 * If we're creating a partition that's a foreign table, verify that
+		 * the parent table is not in a publication with
+		 * publish_via_partition_root enabled.  For details, see
+		 * publication_check_foreign_parts.
+		 */
+		if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
+		{
+			Oid			schemaid;
+			List	   *puboids;
+			List	   *ancestors;
+
+			/* Start with publications of all tables */
+			puboids = GetAllTablesPublications();
+
+			/* capture all publications that include this relation directly */
+			puboids = GetRelationPublications(parent->rd_id);
+			schemaid = RelationGetNamespace(parent);
+			puboids = list_concat(puboids, GetSchemaPublications(schemaid));
+
+			/* and do the same for its ancestors, if any */
+			ancestors = get_partition_ancestors(parent->rd_id);
+			foreach_oid(ancestor, ancestors)
+			{
+				puboids = list_concat(puboids, GetRelationPublications(ancestor));
+				schemaid = get_rel_namespace(ancestor);
+				puboids = list_concat(puboids, GetSchemaPublications(schemaid));
+			}
+
+			/* 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)
+			{
+				Publication *pub = GetPublication(puboid);
+
+				if (pub->pubviaroot)
+					ereport(ERROR,
+							errcode(ERRCODE_WRONG_OBJECT_TYPE),
+							errmsg("cannot create foreign table \"%s\" as a partition of \"%s\"",
+								   RelationGetRelationName(rel), RelationGetRelationName(parent)),
+							errdetail("Partitioned table \"%s\" is published with option \"%s\" in publication \"%s\".",
+									  RelationGetRelationName(parent),
+									  "publish_via_partition_root", pub->name));
+			}
+		}
+
 		/*
 		 * The partition constraint of the default partition depends on the
 		 * partition bounds of every other partition. It is possible that
@@ -20093,58 +20140,68 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
 				 errmsg("cannot attach temporary relation of another session as partition")));
 
 	/*
-	 * Check if attachrel is a foreign table or a partitioned table with
-	 * foreign partition and rel is not part of publication with option
-	 * publish_via_partition_root as true.
+	 * If the relation to attach is a foreign table, or a partitioned table
+	 * that contains a foreign table as partition, then verify that the
+	 * parent table is not in a publication with publish_via_partition_root
+	 * enabled.  See publication_check_foreign_parts.
 	 */
 	if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
-		check_partrel_has_foreign_table(attachrel))
+		(attachrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+		 RelationHasForeignPartition(attachrel)))
 	{
-		Oid			schemaid = RelationGetNamespace(rel);
-		List	   *puboids = GetRelationPublications(rel->rd_id);
+		Oid			schemaid;
+		List	   *puboids;
 		List	   *ancestors;
-		char	   *relname = get_rel_name(rel->rd_id);
-		char	   *attachrelname = get_rel_name(attachrel->rd_id);
 
-		puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
-		puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid));
+		/* Start with publications of all tables */
+		puboids = GetAllTablesPublications();
+
+		/* 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_unique_oid(puboids,
-											 GetRelationPublications(ancestor));
+			puboids = list_concat(puboids, GetRelationPublications(ancestor));
 			schemaid = get_rel_namespace(ancestor);
-			puboids = list_concat_unique_oid(puboids,
-											 GetSchemaPublications(schemaid));
+			puboids = list_concat(puboids, GetSchemaPublications(schemaid));
 		}
 
-		list_free(ancestors);
-
+		/* 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)
 		{
-			Publication *pub = GetPublication(puboid);
+			Publication *pub;
 
+			pub = GetPublication(puboid);
 			if (pub->pubviaroot)
 			{
 				if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
 					ereport(ERROR,
 							(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 							 errmsg("cannot attach foreign table \"%s\" to partition table \"%s\"",
-									attachrelname, relname),
-							 errdetail("partition table \"%s\" is published with option publish_via_partition_root",
-									   relname)));
+									RelationGetRelationName(attachrel),
+									RelationGetRelationName(rel)),
+							 errdetail("Partitioned table \"%s\" is published with option \"%s\" in publication \"%s\".",
+									   RelationGetRelationName(rel),
+									   "publish_via_partition_root",
+									   pub->name)));
 				else
 					ereport(ERROR,
 							(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 							 errmsg("cannot attach table \"%s\" with foreign partition to partition table \"%s\"",
-									attachrelname, relname),
-							 errdetail("partition table \"%s\" is published with option publish_via_partition_root",
-									   relname)));
+									RelationGetRelationName(attachrel),
+									RelationGetRelationName(rel)),
+							 errdetail("Partitioned table \"%s\" is published with option \"%s\" in publication \"%s\".",
+									   RelationGetRelationName(rel),
+									   "publish_via_partition_root",
+									   pub->name)));
 			}
 		}
-
-		list_free(puboids);
 	}
 
 	/*
diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index 328b4d450e4..b53139bafdd 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -506,3 +506,35 @@ get_default_oid_from_partdesc(PartitionDesc partdesc)
 
 	return InvalidOid;
 }
+
+/*
+ * Return true if the given partitioned table ultimately contains a
+ * partition that is a foreign table, false otherwise.
+ */
+bool
+RelationHasForeignPartition(Relation rel)
+{
+	PartitionDesc pd = RelationGetPartitionDesc(rel, true);
+
+	for (int i = 0; i < pd->nparts; i++)
+	{
+		if (pd->is_leaf[i])
+		{
+			if (get_rel_relkind(pd->oids[i]) == RELKIND_FOREIGN_TABLE)
+				return true;
+		}
+		else
+		{
+			Relation	part;
+			bool		ret;
+
+			part = table_open(pd->oids[i], AccessShareLock);
+			ret = RelationHasForeignPartition(part);
+			table_close(part, NoLock);
+			if (ret)
+				return true;
+		}
+	}
+
+	return false;
+}
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index e2919da2541..71ad5a6f846 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -179,6 +179,8 @@ extern Oid	GetTopMostAncestorInPublication(Oid puboid, List *ancestors,
 
 extern bool is_publishable_relation(Relation rel);
 extern bool is_schema_publication(Oid pubid);
+extern void publication_check_foreign_parts(Oid schemaid, char *pubname);
+
 extern bool check_and_fetch_column_list(Publication *pub, Oid relid,
 										MemoryContext mcxt, Bitmapset **cols);
 extern ObjectAddress publication_add_relation(Oid pubid, PublicationRelInfo *pri,
@@ -192,8 +194,4 @@ extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols,
 extern Bitmapset *pub_form_cols_map(Relation relation,
 									PublishGencolsType include_gencols_type);
 
-extern bool check_partrel_has_foreign_table(Relation rel);
-
-extern void check_foreign_tables(Oid schemaid, char *pubname);
-
 #endif							/* PG_PUBLICATION_H */
diff --git a/src/include/partitioning/partdesc.h b/src/include/partitioning/partdesc.h
index 34533f7004c..5fbafdc06f9 100644
--- a/src/include/partitioning/partdesc.h
+++ b/src/include/partitioning/partdesc.h
@@ -71,5 +71,6 @@ extern PartitionDesc PartitionDirectoryLookup(PartitionDirectory, Relation);
 extern void DestroyPartitionDirectory(PartitionDirectory pdir);
 
 extern Oid	get_default_oid_from_partdesc(PartitionDesc partdesc);
+extern bool RelationHasForeignPartition(Relation rel);
 
 #endif							/* PARTDESC_H */
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 6a0c245bd60..4395563fcd5 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1939,32 +1939,32 @@ ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10);
 -- Can't create publications with publish_via_partition_root = true, if table
 -- has a foreign partition
 CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root);
-ERROR:  cannot set parameter "publish_via_partition_root" to true for publication "pub1"
-DETAIL:  partition table "tmain" in publication contains a foreign partition
+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.
 CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root);
-ERROR:  cannot set parameter "publish_via_partition_root" to true for publication "pub1"
-DETAIL:  partition table "tmain" in publication contains a foreign partition
+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 PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root);
-ERROR:  cannot set parameter "publish_via_partition_root" to true for publication "pub1"
-DETAIL:  partition table "tmain" in publication contains a foreign partition
+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.
 -- Test when a partitioned table with foreign table as a partition is attached
 -- to partitioned table which is already published
 ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2;
 CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root);
 ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10);
 ERROR:  cannot attach table "part2" with foreign partition to partition table "tmain"
-DETAIL:  partition table "tmain" is published with option publish_via_partition_root
+DETAIL:  Partitioned table "tmain" is published with option "publish_via_partition_root" in publication "pub1".
 -- Can't create foreign partition of published table with
 -- publish_via_partition_root = true
 CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server;
-ERROR:  cannot create table foreign partition "part3_1"
-DETAIL:  partition table "tmain" is published with option publish_via_partition_root
+ERROR:  cannot create foreign table "part3_1" as a partition of "tmain"
+DETAIL:  Partitioned table "tmain" is published with option "publish_via_partition_root" in publication "pub1".
 -- Can't attach foreign partition to published table with
 -- publish_via_partition_root = true
 CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server;
 ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20);
 ERROR:  cannot attach foreign table "part3_2" to partition table "tmain"
-DETAIL:  partition table "tmain" is published with option publish_via_partition_root
+DETAIL:  Partitioned table "tmain" is published with option "publish_via_partition_root" in publication "pub1".
 CREATE SCHEMA sch4;
 CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id);
 -- publication created with FOR TABLES IN SCHEMA
@@ -1973,13 +1973,13 @@ CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_ro
 -- Can't create foreign partition of published table with
 -- publish_via_partition_root = true
 CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server;
-ERROR:  cannot create table foreign partition "part3_1"
-DETAIL:  partition table "tmain" is published with option publish_via_partition_root
+ERROR:  cannot create foreign table "part3_1" as a partition of "tmain"
+DETAIL:  Partitioned table "tmain" is published with option "publish_via_partition_root" in publication "pub1".
 -- Can't attach foreign partition to published table with
 -- publish_via_partition_root = true
 ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20);
 ERROR:  cannot attach foreign table "part3_2" to partition table "tmain"
-DETAIL:  partition table "tmain" is published with option publish_via_partition_root
+DETAIL:  Partitioned table "tmain" is published with option "publish_via_partition_root" in publication "pub1".
 DROP PUBLICATION pub1;
 -- Test with publish_via_partition_root = false
 -- Foreign partitions are skipped by default
@@ -2004,8 +2004,8 @@ SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3'
 -- Can't alter publish_via_partition_root to true, if publication already have
 -- foreign partition
 ALTER PUBLICATION pub1 SET (publish_via_partition_root);
-ERROR:  cannot set parameter "publish_via_partition_root" to true for publication "pub1"
-DETAIL:  partition table "tmain" in publication contains a foreign partition
+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.
 DROP PUBLICATION pub1;
 DROP PUBLICATION pub2;
 DROP PUBLICATION pub3;
-- 
2.39.5

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 7 Apr 2025 12:05:07 +0200

As promised, here's a rundown of the changes I did, mostly in order the
patch shows them:

- I reworded the documentation changes to read more coherent with the
surrounding text.

- It seemed wrong to have check_publication_add_relation() have the
relation first as argument and publication later, so I turned them
around.

- I rewrote almost all of the error messages.  The errdetails failed
style (no initial capitals and no end period); one of the errmsg() was
completely wrong (it said "cannot set parameter" instead of "cannot add
table to publication").  I also moved the parameter name out of the
translatable part of the message.

- I removed the novel use of the term "foreign partition".  We don't
seem to use that anywhere that's user-visible, only in code comments.
Instead I used "a partition that's a foreign table".  I just did a new
grep and I think I missed a couple, which I'm going to fix momentarily.

- I renamed check_foreign_tables to publication_check_foreign_parts and
moved to a different place in the file.  The original name wasn't very
good for an extern function.

- I renamed check_partrel_has_foreign_table to RelationHasForeignPartition 
and moved it to partdesc.c.  There's nothing that ties this function to
pg_publication.c, so in my mind it made no sense for it to be there.
I'm not 100% certain that partdesc.c is the best place for it, but if
there's another, then I didn't see it.  Also, I made it Assert() that
it's not given a relation other than a partitioned table, which forces
all callers to check the relkind before calling it.  This is good
because we don't need to open tables before ensuring the relkind is one
that interests us.  Also, I made it use the PartitionDesc->is_leaf flag
array to determine whether to recurse or not; and use get_rel_relkind to
get the relkind for leaf partitions, which is much cheaper than doing
table_open.  The original coding was kind of wasteful for no reason.

- The check to prohibit CREATE FOREIGN TABLE as a partition in
CreateForeignTable, was entirely in the wrong place.  This is proven by
the fact that the original coding had to do table_openrv() in order to
know what the parent table was.  This is terrible coding, because it
means we resolve the table name to OID twice.  (This is probably not so
terrible because the first location that does it already acquires a good
enough lock that it'd stay put; still, it's a bad coding pattern).  I
moved it to DefineRelation, in the block were we learn that we're
creating a partition; it's easy to check there that the new relation is
a foreign table.  We don't even need to open the parent table at all,
since it's already open.

- In ATExecAttachPartition (and in the check that was in
CreateForeignTable), there were a bunch of list_concat_unique_oids().
It's unlikely that this would bother anyone unless there are large
numbers of publications, but the performance characteristics of doing
this uniquification were unclear, and list_concat_unique() comments warn
against it anyway.  It seemed safer to just do list_concat() and a
single sort/uniq step at the end, just before walking the list.

- There were a bunch of list_frees() in there.  I removed them all.
They are pointless, unnecessary, and give a wrong sense of cleanliness.
In reality, you will be leaking the second argument of each
list_concat() anyway; to make this watertight, you'd have to read each
of those lists into a variable, concat, then free the variable.  The
code would become much noisier coding.  However, DDL code is normally
leaky and that causes no major problems, because the memory context in
use is going to be released or deleted soon afterwards.  Freeing a few
bytes ahead of time would only require more memory context bookkeeping
to be executed, to very little gain.

I think that's all I had ...

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/