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

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

On Thu, 20 Feb 2025 at 21:18, vignesh C <[email-masked]> wrote:
>
> On Tue, 18 Feb 2025 at 15:59, Shlok Kyal <[email-masked]> wrote:
> >
> > On Mon, 17 Feb 2025 at 20:13, vignesh C <[email-masked]> wrote:
> > >
> > > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <[email-masked]> wrote:
> > > >
> > > > I have used the changes suggested by you. Also I have updated the
> > > > 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.
> > >
> >
> > 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.
>
> 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?

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.

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 1 Apr 2025 20:18:21 +0530

> section about foreign tables, which should be expanded to explain this.

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.

Thanks and Regards,
Shlok Kyal

---

From: Sergey Tatarintsev <[email-masked]>
Date: Fri, 4 Apr 2025 12:06:49 +0700

> I have attached the updated v11 patch.

Hi!

I looked at the latest version of the patch, and think that we should 
free puboids list here:

diff --git a/src/backend/commands/tablecmds.c 
b/src/backend/commands/tablecmds.c
index 6a128f7bd4e..4254654cc24 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -20122,6 +20122,7 @@ ATExecAttachPartition(List **wqueue, Relation 
rel, PartitionCmd *cmd,
relname)));
                         }
                 }
+               list_free(puboids);
         }

         /*

-- 
With best regards,
Sergey Tatarintsev,
PostgresPro

---

From: Shlok Kyal <[email-masked]>
Date: Fri, 4 Apr 2025 14:44:17 +0530

> @@ -20122,6 +20122,7 @@ ATExecAttachPartition(List **wqueue, Relation

Hi Sergey,

Thanks for reviewing the patch.

I have fixed the comment. I also found other places where we should
free the relids, schemaoids, and puboids list. I have added changes
for those as well.
I have attached the updated patch.

Thanks and Regards,
Shlok Kyal