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

From: Shlok Kyal <[email-masked]>
Date: Fri, 14 Feb 2025 12:59:05 +0530

On Thu, 13 Feb 2025 at 20:12, vignesh C <[email-masked]> wrote:
>
> 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.
>

Hi Vignesh,

I have used the changes suggested by you. Also I have updated the
comments and the function name.

Thanks and Regards,
Shlok Kyal

---

From: vignesh C <[email-masked]>
Date: Mon, 17 Feb 2025 20:12:49 +0530

> comments and the function name.

There is another concurrency issue possible:
+/* Check if a partitioned table has a foreign partition */
+bool
+check_partrel_has_foreign_table(Form_pg_class relform)
+{
+       bool            has_foreign_tbl = false;
+
+       if (relform->relkind == RELKIND_PARTITIONED_TABLE)
+       {
+               List       *relids = NIL;
+
+               relids = find_all_inheritors(relform->oid, NoLock, NULL);

Create a publication with publish_via_partition_root as true, hold the
execution after check_partrel_has_foreign_table execution finishes.
Then parallely execute the following:
CREATE TABLE t1(id int) PARTITION BY RANGE(id);
CREATE TABLE part1 PARTITION OF t1 FOR VALUES FROM (0) TO (5);
CREATE TABLE part2 PARTITION OF t1 FOR VALUES FROM (5) TO (15)
PARTITION BY RANGE(id);
CREATE FOREIGN TABLE part2_1 PARTITION OF part2 FOR VALUES FROM (10)
TO (15) SERVER fdw

Now both the partitioned table having foreign table and a publication
will be created.

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 18 Feb 2025 15:59:43 +0530

Hi Vignesh,

I have addressed the above issue. If we take a ShareLock on the
pg_class, we won't be able to create table concurrently, which may
address the issue. Thoughts?
I have attached the v8 patch here.

Thanks and Regards,
Shlok Kyal

---

From: vignesh C <[email-masked]>
Date: Thu, 20 Feb 2025 21:18:10 +0530

> I have attached the v8 patch here.

Since pg_class is a very common table it is not a good idea to take
ShareLock on it. Will it be possible to use pg_partitioned_table table
instead?

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Mon, 24 Mar 2025 15:03:14 +0530

Hi Vignesh,

I have addressed comments and I have modified the patch to take a
ShareLock on pg_partitioned_table table.
Please find the updated v9 patch.

Thanks and Regards,
Shlok Kyal

---

From: Álvaro Herrera <[email-masked]>
Date: Mon, 24 Mar 2025 16:47:31 +0100

One thing that bothers me a bit about this is that there's no single
code comment where this restriction it documented in full; in fact it
doesn't seem documented anywhere, only in the commit message.

I think check_foreign_tables() is a good place to add an explanatory
comment; other places can reference that.  For instance, add something
like

/*
 * Protect against including foreign tables that are partitions of
 * partitioned tables published by the given publication.  This would
 * not work properly, because <!-- explain reason -->, so we disallow
 * the case here and in all DDL commands that would end up creating
 * such a case indirectly.
 */

Then for instance in check_publication_add_relation() and
ATExecAttachPartition() you comment would say /* if the would-be
partition is a foreign table, verify that the partitioned table is not
in a publication with publish_via_root=false.  See check_foreign_tables
for details */

Also, surely we should document this restriction in the SGML docs
somewhere.

Would it be better if check_partrel_has_foreign_table() used
RelationGetPartitionDesc(omit_detached=true) instead of
find_all_inheritors()?

I'm wary of all those accesses of subscription/publication catalogs in
DDL code.  Maybe I worry about nothing, but I cannot but feel that we're
missing one layer of abstraction there (including possibly some caching
on top of syscache).

I think this
castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations)));
is better written
linitial_node(RangeVar, stmt->base.inhRelations);

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 28 Mar 2025 15:13:44 +0530

> in a publication with publish_via_root=false.  See check_foreign_tables

I have added comments as suggested above.

> Also, surely we should document this restriction in the SGML docs
I have added comment in create_publication.sgml

> missing one layer of abstraction there (including possibly some caching
I also think using RelationGetPartitionDesc would be better and made the change.

> linitial_node(RangeVar, stmt->base.inhRelations);

Fixed.

I have attached the updated v10 patch with the above changes.

Thanks and Regards,
Shlok Kyal

---

From: Álvaro Herrera <[email-masked]>
Date: Fri, 28 Mar 2025 12:04:57 +0100

> I have added comment in create_publication.sgml

Hmm, okay, but "We cannot" is not the style used in the documentation.
In addition, I think this mechanism should be mentioned in
logical-replication.sgml; currently there's a note in the Restrictions
section about foreign tables, which should be expanded to explain this.