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

From: vignesh C <[email-masked]>
Date: Thu, 13 Feb 2025 20:11:48 +0530

On Thu, 13 Feb 2025 at 15:50, Shlok Kyal <[email-masked]> wrote:
>
>
> I have fixed the issue. Attached the updated v6 patch.

There is another concurrency issue:
In case of create publication for all tables with
publish_via_partition_root we will call check_foreign_tables:
@@ -876,6 +876,10 @@ 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 */
+               if (publish_via_partition_root)
+                       check_foreign_tables(stmt->pubname);

At the time of check in check_foreign_tables, there are no foreign
tables so this check will be successful:
+check_foreign_tables_in_schema(Oid schemaid, char *pubname)
+{
+       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)

Now immediately after execution of this, create a foreign table:
postgres=# CREATE FOREIGN TABLE part22 PARTITION OF part2 FOR VALUES
FROM (10) TO (15) SERVER fdw;
CREATE FOREIGN TABLE

And then continue execution of create publication, it will also be successful:
postgres=# create publication pub1 for all tables with (
publish_via_partition_root =true);
CREATE PUBLICATION

One probable way to fix this is to do the search similar to
check_foreign_tables_in_schema where we can skip including schemaid
key for all tables.

How about something like the attached patch.

Regards,
Vignesh
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 520fa2f382..3214104dec 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1388,21 +1388,24 @@ check_foreign_tables_in_schema(Oid schemaid, char *pubname)
 {
 	Relation	classRel;
 	ScanKeyData key[2];
+	int 		keycount = 0;
 	TableScanDesc scan;
 	HeapTuple	tuple;
 
 	classRel = table_open(RelationRelationId, AccessShareLock);
 
-	ScanKeyInit(&key[0],
-				Anum_pg_class_relnamespace,
-				BTEqualStrategyNumber, F_OIDEQ,
-				schemaid);
-	ScanKeyInit(&key[1],
+	ScanKeyInit(&key[keycount++],
 				Anum_pg_class_relkind,
 				BTEqualStrategyNumber, F_CHAREQ,
 				CharGetDatum(RELKIND_PARTITIONED_TABLE));
-
-	scan = table_beginscan_catalog(classRel, 2, key);
+	
+	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);
@@ -1436,46 +1439,3 @@ check_foreign_tables_in_schema(Oid schemaid, char *pubname)
 	table_endscan(scan);
 	table_close(classRel, AccessShareLock);
 }
-
-/* Check if any foreign table is a partition table */
-void
-check_foreign_tables(char *pubname)
-{
-	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 set parameter \"%s\" to true for publication \"%s\"",
-							"publish_via_partition_root", pubname),
-					 errdetail("partition table \"%s\" in publication contains a foreign partition",
-							   parent_name)));
-		}
-	}
-
-	table_endscan(scan);
-	table_close(classRel, AccessShareLock);
-}
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index e71d5408c7..b8da4ed130 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -878,7 +878,7 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt)
 	{
 		/* Check if any foreign table is a part of partitioned table */
 		if (publish_via_partition_root)
-			check_foreign_tables(stmt->pubname);
+			check_foreign_tables_in_schema(InvalidOid, stmt->pubname);
 
 		/* Invalidate relcache so that publication info is rebuilt. */
 		CacheInvalidateRelcacheAll();
@@ -1057,7 +1057,7 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt,
 		char	   *pubname = stmt->pubname;
 
 		if (pubform->puballtables)
-			check_foreign_tables(pubname);
+			check_foreign_tables_in_schema(InvalidOid, pubname);
 
 		schemaoids = GetPublicationSchemas(pubform->oid);
 
diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h
index b1bb864d2f..fcd428465e 100644
--- a/src/include/catalog/pg_publication.h
+++ b/src/include/catalog/pg_publication.h
@@ -196,6 +196,4 @@ extern bool check_partrel_has_foreign_table(Form_pg_class relform);
 
 extern void check_foreign_tables_in_schema(Oid schemaid, char *pubname);
 
-extern void check_foreign_tables(char *pubname);
-
 #endif							/* PG_PUBLICATION_H */