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

From: Shlok Kyal <[email-masked]>
Date: Thu, 30 Jan 2025 17:32:30 +0530

On Wed, 29 Jan 2025 at 19:21, Sergey Tatarintsev
<[email-masked]> wrote:
>
>
> 29.01.2025 12:16, Shlok Kyal пишет:
> > Hi,
> >
> > As part of a discussion in [1], I am starting this thread to address
> > the issue reported for foreign tables.
> >
> > Logical replication of foreign tables is not supported, and we throw
> > an error in this case. But when we create a publication on a
> > partitioned table that has a foreign table as a partition, the initial
> > sync of such a table is successful. We should also throw an error in
> > such cases.
> > With this patch, we will throw an error when we try to create a
> > publication on (or add to an existing publication) a partitioned table
> > with a foreign table as its partition or attach such a table to
> > existing published tables.
> >
> > [1] : https://www.postgresql.org/message-id/CAA4eK1Lhh4SgiYQLNiWSNKGdVSzbd53%3Dsr2tQCKooEphDkUtgw%40mail.gmail.com
> >
> > Thanks and Regards,
> > Shlok Kyal
> Hi!
>
> Thanks for patch.
>
> I reviewed it and made some changes.
>
> 1. we should check foreign tables (not partitioned)
> 2. added checking for foreign table creation
> 3. some little corrections
>
> See attach
>

Hi Sergey,

I have added most of the changes in v2-0002 with small changes except
one change.

@@ -1428,6 +1427,12 @@ check_foreign_tables_in_schema(Oid schemaid)
                     errdetail("foreign table \"%s\" is a partition of
partitioned table \"%s\"",
                               get_rel_name(foreign_tbl_relid), parent_name)));
        }
+       else if (relForm->relkind == RELKIND_FOREIGN_TABLE)
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                    errmsg("cannot add relation \"%s\" to publication",
+                           get_rel_name(relForm->oid)),
+                    errdetail_relkind_not_supported(RELKIND_FOREIGN_TABLE)));
    }

We should only throw error when foreign table is part of a partition
table in case of 'FOR TABLES IN SCHEMA' . We should not throw an error
otherwise because in case of 'FOR TABLES IN SCHEMA' foreign tables are
not published by default.

I have added the changes in v3-0001.

Thanks and Regards,
Shlok Kyal

---

From: Sergey Tatarintsev <[email-masked]>
Date: Fri, 31 Jan 2025 12:08:09 +0700

> I have added the changes in v3-0001.

Hello!

Ok, but maybe it will be correct to raise an WARNING (or at least LOG) 
that some tables was skipped during publication. What do you think?

And I think we need check tables which was really published in case of 
'FOR ALL TABLES' and 'FOR TABLES IN SCHEMA' in our tests. See attach.

Also it looks strange - we raise an ERROR for partitioned tables 
containing foreign partitions , but just skip foreign tables as itself. 
I think there should be the same behavior in both cases - raise an ERROR 
or skip.

From 349750385cd3df12c02654a91997f48f2abd9832 Mon Sep 17 00:00:00 2001
From: Shlok Kyal <[email-masked]>
Date: Wed, 29 Jan 2025 10:18:53 +0530
Subject: [PATCH v3] Restrict publishing of partitioned table with a foreign
 table as partition

Logical replication of foreign table is not supported and we throw an
error in this case. But when create a publication on a partitioned
table that has a foreign table as partition, the initial sync of such
table is successful. We should also throw an error in such cases.
With this patch we will throw an error when we try create a publication
on (or add to existing publication) a partitioned table with foreign
table as its partition. We will also throw an error when we try to
attach such table to existing published tables.
---
 src/backend/catalog/pg_publication.c      | 151 ++++++++++++++++++++++
 src/backend/commands/foreigncmds.c        |  40 ++++++
 src/backend/commands/publicationcmds.c    |   3 +
 src/backend/commands/tablecmds.c          |  44 +++++++
 src/include/catalog/pg_publication.h      |   8 ++
 src/test/regress/expected/publication.out |  39 ++++++
 src/test/regress/sql/publication.sql      |  37 ++++++
 7 files changed, 322 insertions(+)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 41ffd494c8..071b428bc0 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -55,6 +55,8 @@ typedef struct
 static void
 check_publication_add_relation(Relation targetrel)
 {
+	Oid			foreign_tbl_relid;
+
 	/* Must be a regular or partitioned table */
 	if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION &&
 		RelationGetForm(targetrel)->relkind != RELKIND_PARTITIONED_TABLE)
@@ -64,6 +66,19 @@ check_publication_add_relation(Relation targetrel)
 						RelationGetRelationName(targetrel)),
 				 errdetail_relkind_not_supported(RelationGetForm(targetrel)->relkind)));
 
+	/*
+	 * Check if it is a partitioned table and any foreign table is its
+	 * partition
+	 */
+	if (check_partrel_has_foreign_table(RelationGetForm(targetrel), &foreign_tbl_relid))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("cannot add relation \"%s\" to publication",
+						RelationGetRelationName(targetrel)),
+				 errdetail("foreign table \"%s\" is a partition of partitioned table \"%s\"",
+						   get_rel_name(foreign_tbl_relid),
+						   RelationGetRelationName(targetrel))));
+
 	/* Can't be system table */
 	if (IsCatalogRelation(targetrel))
 		ereport(ERROR,
@@ -695,6 +710,9 @@ publication_add_schema(Oid pubid, Oid schemaid, bool if_not_exists)
 
 	check_publication_add_schema(schemaid);
 
+	/* check if schema has any foreign table as partition table */
+	check_foreign_tables_in_schema(schemaid);
+
 	/* Form a tuple */
 	memset(values, 0, sizeof(values));
 	memset(nulls, false, sizeof(nulls));
@@ -1324,3 +1342,136 @@ 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(Form_pg_class relform, Oid *foreign_tbl_relid)
+{
+	bool		has_foreign_tbl = false;
+
+	*foreign_tbl_relid = InvalidOid;
+	if (relform->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		List	   *relids = NIL;
+
+		relids = GetPubPartitionOptionRelations(relids, PUBLICATION_PART_LEAF,
+												relform->oid);
+
+		foreach_oid(relid, relids)
+		{
+			Relation	rel = table_open(relid, AccessShareLock);
+
+			if (RelationGetForm(rel)->relkind == RELKIND_FOREIGN_TABLE)
+			{
+				has_foreign_tbl = true;
+				*foreign_tbl_relid = relid;
+			}
+
+			table_close(rel, AccessShareLock);
+
+			if (has_foreign_tbl)
+				break;
+		}
+	}
+
+	return has_foreign_tbl;
+}
+
+/* Check if a schema has a partitioned table which has a foreign partition */
+void
+check_foreign_tables_in_schema(Oid schemaid)
+{
+	Relation	classRel;
+	ScanKeyData key[2];
+	TableScanDesc scan;
+	HeapTuple	tuple;
+
+	classRel = table_open(RelationRelationId, AccessShareLock);
+
+	ScanKeyInit(&key[0],
+				Anum_pg_class_relnamespace,
+				BTEqualStrategyNumber, F_OIDEQ,
+				schemaid);
+	ScanKeyInit(&key[1],
+				Anum_pg_class_relkind,
+				BTEqualStrategyNumber, F_CHAREQ,
+				CharGetDatum(RELKIND_PARTITIONED_TABLE));
+
+	scan = table_beginscan_catalog(classRel, 2, key);
+	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+	{
+		Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+		Oid			foreign_tbl_relid;
+
+		if (check_partrel_has_foreign_table(relForm, &foreign_tbl_relid))
+		{
+			List	   *ancestors = get_partition_ancestors(relForm->oid);
+			Oid			parent_oid = relForm->oid;
+			char	   *parent_name;
+
+			foreach_oid(ancestor, ancestors)
+			{
+				Oid			ancestor_schemaid = get_rel_namespace(ancestor);
+
+				if (ancestor_schemaid == schemaid)
+					parent_oid = ancestor;
+			}
+
+			list_free(ancestors);
+			parent_name = get_rel_name(parent_oid);
+
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("cannot add relation \"%s\" to publication",
+							parent_name),
+					 errdetail("foreign table \"%s\" is a partition of partitioned table \"%s\"",
+							   get_rel_name(foreign_tbl_relid), parent_name)));
+		}
+	}
+
+	table_endscan(scan);
+	table_close(classRel, AccessShareLock);
+}
+
+/* Check if any foreign table is a partition table */
+void
+check_foreign_tables(void)
+{
+	Relation	classRel;
+	ScanKeyData key[1];
+	TableScanDesc scan;
+	HeapTuple	tuple;
+
+	classRel = table_open(RelationRelationId, AccessShareLock);
+
+	ScanKeyInit(&key[0],
+				Anum_pg_class_relkind,
+				BTEqualStrategyNumber, F_CHAREQ,
+				CharGetDatum(RELKIND_FOREIGN_TABLE));
+
+	scan = table_beginscan_catalog(classRel, 1, key);
+	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+	{
+		Form_pg_class relForm = (Form_pg_class) GETSTRUCT(tuple);
+
+		if (relForm->relispartition)
+		{
+			Oid			parent_oid;
+			char	   *parent_name;
+			List	   *ancestors = get_partition_ancestors(relForm->oid);
+
+			parent_oid = llast_oid(ancestors);
+			parent_name = get_rel_name(parent_oid);
+
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("cannot add relation \"%s\" to publication",
+							parent_name),
+					 errdetail("foreign table \"%s\" is a partition of partitioned table \"%s\"",
+							   NameStr(relForm->relname), parent_name)));
+		}
+	}
+
+	table_endscan(scan);
+	table_close(classRel, AccessShareLock);
+}
diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c
index c14e038d54..004edd9515 100644
--- a/src/backend/commands/foreigncmds.c
+++ b/src/backend/commands/foreigncmds.c
@@ -21,6 +21,7 @@
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
 #include "catalog/objectaccess.h"
+#include "catalog/partition.h"
 #include "catalog/pg_foreign_data_wrapper.h"
 #include "catalog/pg_foreign_server.h"
 #include "catalog/pg_foreign_table.h"
@@ -1423,6 +1424,45 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid)
 
 	ftrel = table_open(ForeignTableRelationId, RowExclusiveLock);
 
+	/*
+	 * Check if it is a foreign partition and the partitioned table is
+	 * published
+	 */
+	if (stmt->base.partbound != NULL)
+	{
+		RangeVar   *root = castNode(RangeVar, lfirst(list_head(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);
+
+			if (puboids)
+				ereport(ERROR,
+						(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+						 errmsg("cannot create foreign partition \"%s\" as partitioned table \"%s\" is published",
+								get_rel_name(relid),
+								RelationGetRelationName(rootrel))));
+		}
+		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 951ffabb65..7258d6c33d 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -855,6 +855,9 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
 	/* Associate objects with the publication. */
 	if (stmt->for_all_tables)
 	{
+		/* Check if any foreign table is a part of partitioned table */
+		check_foreign_tables();
+
 		/* Invalidate relcache so that publication info is rebuilt. */
 		CacheInvalidateRelcacheAll();
 	}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d617c4bc63..82e07509a8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -19222,6 +19222,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
 	Oid			defaultPartOid;
 	List	   *partBoundConstraint;
 	ParseState *pstate = make_parsestate(NULL);
+	Oid			foreign_tbl_relid;
 
 	pstate->p_sourcetext = context->queryString;
 
@@ -19347,6 +19348,49 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd,
 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
 				 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 published.
+	 */
+	if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE ||
+		check_partrel_has_foreign_table(RelationGetForm(attachrel), &foreign_tbl_relid))
+	{
+		Oid			schemaid = RelationGetNamespace(rel);
+		List	   *puboids = GetRelationPublications(rel->rd_id);
+		List	   *ancestors;
+
+		puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
+		puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid));
+		ancestors = get_partition_ancestors(rel->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);
+
+		if (puboids)
+		{
+			if (attachrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+				ereport(ERROR,
+						(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+						 errmsg("cannot attach a partitioned table with a foreign partition to a published table"),
+						 errdetail("foreign table \"%s\" is a partition of partitioned table \"%s\"",
+								   get_rel_name(foreign_tbl_relid),
+								   RelationGetRelationName(attachrel))));
+			else
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("cannot attach foreign table \"%s\" to a published table",
+								get_rel_name(attachrel->rd_id))));
+
+		}
+	}
+
 	/*
 	 * Check if attachrel has any identity columns or any columns that aren't
 	 * in the parent.
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index 48c7d1a861..ba51f4a721 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -19,6 +19,7 @@
 
 #include "catalog/genbki.h"
 #include "catalog/objectaddress.h"
+#include "catalog/pg_class.h"
 #include "catalog/pg_publication_d.h"	/* IWYU pragma: export */
 
 /* ----------------
@@ -191,4 +192,11 @@ 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(Form_pg_class relform,
+											Oid *foreign_tbl_name);
+
+extern void check_foreign_tables_in_schema(Oid schemaid);
+
+extern void check_foreign_tables(void);
+
 #endif							/* PG_PUBLICATION_H */
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index bc3898fbe5..b973f41e94 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1885,6 +1885,45 @@ DROP PUBLICATION pub1;
 DROP PUBLICATION pub2;
 DROP TABLE gencols;
 RESET client_min_messages;
+-- ======================================================
+-- Test when foreign table is a partition of a partitioned table on which
+-- publication is created
+SET client_min_messages = 'ERROR';
+CREATE FOREIGN DATA WRAPPER test_fdw;
+CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw;
+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);
+CREATE PUBLICATION pub1 FOR TABLE sch3.tmain;
+ERROR:  cannot add relation "tmain" to publication
+DETAIL:  foreign table "part2_1" is a partition of partitioned table "tmain"
+CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3;
+ERROR:  cannot add relation "tmain" to publication
+DETAIL:  foreign table "part2_1" is a partition of partitioned table "tmain"
+CREATE PUBLICATION pub1 FOR ALL TABLES;
+ERROR:  cannot add relation "tmain" to publication
+DETAIL:  foreign table "part2_1" is a partition of partitioned table "tmain"
+-- 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;
+ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10);
+ERROR:  cannot attach a partitioned table with a foreign partition to a published table
+DETAIL:  foreign table "part2_1" is a partition of partitioned table "part2"
+-- Can't create foreign partition of published table
+CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server;
+ERROR:  cannot create foreign partition "part3_1" as partitioned table "tmain" is published
+-- Can't attach foreign partition to published table
+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 a published table
+DROP PUBLICATION pub1;
+DROP SCHEMA sch3 CASCADE;
+DROP SERVER fdw_server;
+DROP FOREIGN DATA WRAPPER test_fdw;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 47f0329c24..b9811033dc 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1186,6 +1186,43 @@ DROP PUBLICATION pub2;
 DROP TABLE gencols;
 
 RESET client_min_messages;
+-- ======================================================
+
+-- Test when foreign table is a partition of a partitioned table on which
+-- publication is created
+SET client_min_messages = 'ERROR';
+CREATE FOREIGN DATA WRAPPER test_fdw;
+CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw;
+
+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);
+
+CREATE PUBLICATION pub1 FOR TABLE sch3.tmain;
+CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3;
+CREATE PUBLICATION pub1 FOR ALL TABLES;
+
+-- 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;
+ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10);
+
+-- Can't create foreign partition of published table
+CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server;
+
+-- Can't attach foreign partition to published table
+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);
+
+DROP PUBLICATION pub1;
+DROP SCHEMA sch3 CASCADE;
+DROP SERVER fdw_server;
+DROP FOREIGN DATA WRAPPER test_fdw;
+
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
-- 
2.34.1

From 8a2a4f94e4c1f35e77179e6de0d4f3f2d1c7cf1f Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <[email-masked]>
Date: Fri, 31 Jan 2025 11:57:18 +0700
Subject: [PATCH] Tests: check tables which was really published via 'FOR ALL
 TABLES' and 'FOR TABLES IN SCHEMA'

---
 src/backend/catalog/pg_publication.c      |  1 +
 src/test/regress/expected/publication.out | 26 ++++++++++++++++++++++-
 src/test/regress/sql/publication.sql      | 21 ++++++++++++++++--
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 071b428bc0..b7be142bd0 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1372,6 +1372,7 @@ check_partrel_has_foreign_table(Form_pg_class relform, Oid *foreign_tbl_relid)
 			if (has_foreign_tbl)
 				break;
 		}
+		list_free(relids);
 	}
 
 	return has_foreign_tbl;
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index b973f41e94..20dfe5c6d0 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -1886,6 +1886,11 @@ DROP PUBLICATION pub2;
 DROP TABLE gencols;
 RESET client_min_messages;
 -- ======================================================
+-- We don't want to see previously created tables, so we'll create an empty database
+SELECT current_database() as prev_dbname
+\gset
+CREATE DATABASE publication_test_db;
+\c publication_test_db
 -- Test when foreign table is a partition of a partitioned table on which
 -- publication is created
 SET client_min_messages = 'ERROR';
@@ -1922,8 +1927,27 @@ ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20
 ERROR:  cannot attach foreign table "part3_2" to a published table
 DROP PUBLICATION pub1;
 DROP SCHEMA sch3 CASCADE;
-DROP SERVER fdw_server;
+CREATE SCHEMA sch4;
+CREATE TABLE sch4.ltable(id int) PARTITION BY RANGE(id);
+CREATE TABLE sch4.ltable_part PARTITION OF sch4.ltable FOR VALUES FROM (0) TO (5);
+CREATE FOREIGN TABLE sch4.ftable(id int) SERVER fdw_server;
+CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4;
+CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root);
+CREATE PUBLICATION pub3 FOR ALL TABLES;
+SELECT pubname, tablename FROM pg_publication_tables ORDER BY pubname, tablename;
+ pubname |  tablename  
+---------+-------------
+ pub1    | ltable_part
+ pub2    | ltable
+ pub3    | ltable_part
+(3 rows)
+
+DROP PUBLICATION pub1, pub2;
+DROP SCHEMA sch4 CASCADE;
+DROP SERVER fdw_server CASCADE;
 DROP FOREIGN DATA WRAPPER test_fdw;
+\c :prev_dbname
+DROP DATABASE publication_test_db;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index b9811033dc..b24c1e90c7 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1188,6 +1188,11 @@ DROP TABLE gencols;
 RESET client_min_messages;
 -- ======================================================
 
+-- We don't want to see previously created tables, so we'll create an empty database
+SELECT current_database() as prev_dbname
+\gset
+CREATE DATABASE publication_test_db;
+\c publication_test_db
 -- Test when foreign table is a partition of a partitioned table on which
 -- publication is created
 SET client_min_messages = 'ERROR';
@@ -1220,9 +1225,21 @@ ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20
 
 DROP PUBLICATION pub1;
 DROP SCHEMA sch3 CASCADE;
-DROP SERVER fdw_server;
-DROP FOREIGN DATA WRAPPER test_fdw;
 
+CREATE SCHEMA sch4;
+CREATE TABLE sch4.ltable(id int) PARTITION BY RANGE(id);
+CREATE TABLE sch4.ltable_part PARTITION OF sch4.ltable FOR VALUES FROM (0) TO (5);
+CREATE FOREIGN TABLE sch4.ftable(id int) SERVER fdw_server;
+CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4;
+CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root);
+CREATE PUBLICATION pub3 FOR ALL TABLES;
+SELECT pubname, tablename FROM pg_publication_tables ORDER BY pubname, tablename;
+DROP PUBLICATION pub1, pub2;
+DROP SCHEMA sch4 CASCADE;
+DROP SERVER fdw_server CASCADE;
+DROP FOREIGN DATA WRAPPER test_fdw;
+\c :prev_dbname
+DROP DATABASE publication_test_db;
 RESET SESSION AUTHORIZATION;
 DROP ROLE regress_publication_user, regress_publication_user2;
 DROP ROLE regress_publication_user_dummy;
-- 
2.43.0

---

From: vignesh C <[email-masked]>
Date: Tue, 4 Feb 2025 18:16:24 +0530

> I think there should be the same behavior in both cases - raise an ERROR

I believe this behavior is acceptable. We should throw an error for
partitioned tables that contain foreign partitions, as this would
include the data from these foreign tables during the initial sync,
while incremental changes would not be replicated. However, in the
case of foreign tables, neither the initial sync nor the incremental
sync will occur. Additionally, it is documented in [1] that foreign
tables are not supported, so this behavior is in line with the
documentation and is acceptable.

[1] - https://www.postgresql.org/docs/current/logical-replication-restrictions.html

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Tue, 4 Feb 2025 18:31:19 +0530

> I have added the changes in v3-0001.

In case of all tables publication you have retrieved all the foreign
tables and then checked if any of the foreign tables is a partition of
a partitioned table. In case of all tables in schema publication you
have retrieved all the partitioned tables and then check if it
includes foreign tables. I felt you can keep the all tables in schema
publication also similar to all tables publication(as generally the
number of foreign tables will be lesser than the tables) i.e. retrieve
all the foreign tables and then check if any of the foreign tables is
a partition of a partitioned table.

Regards,
Vignesh

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 4 Feb 2025 16:51:19 +0100

On 2025-Feb-04, vignesh C wrote:

> We should throw an error for partitioned tables that contain foreign
> partitions, as this would include the data from these foreign tables
> during the initial sync, while incremental changes would not be
> replicated.

Hmm, I would support the idea of allowing partitioned tables containing
foreign partitions into publications, where only the tuples in
non-foreign partitions would be transmitted -- both during initial sync
and during replicated DML.  This way, any partitioned tables with mixed
local and foreign partitions would work okay for replication.  If the
subscriber wants the data in the foreign partitions, they can create the
foreign table on the subscription side and all is well.  From a users
POV this is probably the most useful.

Another aspect is the case where you create the publication first and
the foreign table later.  I didn't spot in the proposed patch any code
that would check whether a publication including this schema exists,
which we would have to do if we wanted to be watertight about rejecting
publications with foreign tables.  But I think that would be a bad
direction to go it.

I'd rather have the table-listing code for publications skip any foreign
tables.  For instance, GetPubPartitionOptionRelations() and
PublicationAddTables() should skip tables with relkind RELKIND_FOREIGN.
The sync code should also silently ignore all foreign tables.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I am amazed at [the pgsql-sql] mailing list for the wonderful support, and
lack of hesitasion in answering a lost soul's question, I just wished the rest
of the mailing list could be like this."                               (Fotis)
              https://postgr.es/m/[email-masked]

---

From: vignesh C <[email-masked]>
Date: Mon, 10 Feb 2025 16:10:56 +0530

> a partition of a partitioned table.

I believe you chose this approach because partitioned tables can have
their partitions as foreign tables in a different schema. As a result,
the current approach works well for the 'TABLES IN SCHEMA' case. It
would be helpful to include a brief comment explaining this reasoning
where you're handling tables in the schema publication.

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 11 Feb 2025 16:56:21 +0530

> where you're handling tables in the schema publication.

Correct, I used this approach because partitioned tables can have
their foreign partitions in a different schema. I have added a comment
for the same in the v5 patch [1].

[1]: https://www.postgresql.org/message-id/CANhcyEXxjq9U7BXxCba3Njz%2BeHpNzAsSVY2GzbV%3D8iy5j%3DUAUA%40mail.gmail.com

Thanks and Regards,
Shlok Kyal