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

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

From

Álvaro Herrera

Date:

04 February, 18:51:19

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/200606261359.k5QDxE2p004593@auth-smtp.hol.gr

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

From

vignesh C

Date:

05 February, 09:42:40

On Tue, 4 Feb 2025 at 21:21, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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.

We can maintain the behavior you suggested when the
PUBLISH_VIA_PARTITION_ROOT option is set to false. However, when
PUBLISH_VIA_PARTITION_ROOT is true, the table data is copied from the
root table (as intended by the user), which will also include the
foreign table data. In this case, wouldn’t it be better to throw an
error?

Regards,
Vignesh

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

From

Álvaro Herrera

Date:

05 February, 11:44:40

On 2025-Feb-05, vignesh C wrote:

> We can maintain the behavior you suggested when the
> PUBLISH_VIA_PARTITION_ROOT option is set to false. However, when
> PUBLISH_VIA_PARTITION_ROOT is true, the table data is copied from the
> root table (as intended by the user), which will also include the
> foreign table data. In this case, wouldn’t it be better to throw an
> error?

It sounds to me a reasonable restriction that you can only add a
partitioned table to a publication if publish_via_partition_root=false.
Then the case of hybrid partitioned tables is supported, but it requires
that changes are published directly by partitions, which is sensible
anyway.

In this case, during CREATE FOREIGN TABLE of a partition with this
condition, we must check whether any publications include the schema
that the table is being created on (or attached, for ALTER TABLE ATTACH
PARTITION), and whether there are any publications that are FOR ALL
TABLES that would be affected.

(If we later figure out a way to allow publish_via_partition_root and
skip the tuples in foreign partitions, it's easy to remove the
restriction.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"That sort of implies that there are Emacs keystrokes which aren't obscure.
I've been using it daily for 2 years now and have yet to discover any key
sequence which makes any sense."                        (Paul Thomas)

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

From

vignesh C

Date:

13 February, 08:54:51

On Tue, 11 Feb 2025 at 16:55, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> I have handled the above cases and added tests for the same.

There is a concurrency issue with the patch:
+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);
+
+               foreach_oid(relid, relids)
+               {
+                       Relation        rel = table_open(relid,
AccessShareLock);
+
+                       if (RelationGetForm(rel)->relkind ==
RELKIND_FOREIGN_TABLE)
+                               has_foreign_tbl = true;
+
+                       table_close(rel, AccessShareLock);
+
+                       if (has_foreign_tbl)
+                               break;
+               }
+       }
+
+       return has_foreign_tbl;
+}

In an ideal scenario, the creation of a foreign table should fail if
there is an associated publication, as demonstrated below:
CREATE TABLE t(id int) PARTITION BY RANGE(id);
CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
CREATE TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (15)
PARTITION BY RANGE(id);
CREATE PUBLICATION pub1 FOR TABLE t with (publish_via_partition_root = true);

postgres=# CREATE FOREIGN TABLE part22 PARTITION OF part2 FOR VALUES
FROM (10) TO (15) SERVER fdw;
ERROR:  cannot create table foreign partition "part22"
DETAIL:  partition table "part2" is published with option
publish_via_partition_root

Consider a scenario where the publication is being created and after
the check_partrel_has_foreign_table execution is done, concurrently
creation of foreign table is executed, then the creation will be
successful.
postgres=# CREATE FOREIGN TABLE part22 PARTITION OF part2 FOR VALUES
FROM (10) TO (15) SERVER fdw;
CREATE FOREIGN TABLE

I felt the problem here is that you have released the lock:
+                       if (RelationGetForm(rel)->relkind ==
RELKIND_FOREIGN_TABLE)
+                               has_foreign_tbl = true;
+
+                       table_close(rel, AccessShareLock);

We should retain the lock to fix this issue:
+                       if (RelationGetForm(rel)->relkind ==
RELKIND_FOREIGN_TABLE)
+                               has_foreign_tbl = true;
+
+                       table_close(rel, NoLock);

Regards,
Vignesh

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

From

vignesh C

Date:

17 February, 17:42:49

On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <shlok.kyal.oss@gmail.com> 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.

Regards,
Vignesh

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

From

vignesh C

Date:

20 February, 18:48:10

On Tue, 18 Feb 2025 at 15:59, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Mon, 17 Feb 2025 at 20:13, vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <shlok.kyal.oss@gmail.com> 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?

Regards,
Vignesh

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

From

Shlok Kyal

Date:

24 March, 12:33:14

On Thu, 20 Feb 2025 at 21:18, vignesh C <vignesh21@gmail.com> wrote:
>
> On Tue, 18 Feb 2025 at 15:59, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Mon, 17 Feb 2025 at 20:13, vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <shlok.kyal.oss@gmail.com> 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

Attachment

v9-0001-Restrict-publishing-of-partitioned-table-with-for.patch

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

From

Álvaro Herrera

Date:

24 March, 18:47:31

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)

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

From

Shlok Kyal

Date:

28 March, 12:43:44

On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> 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 */
>

I have added comments as suggested above.

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

>
> 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 also think using RelationGetPartitionDesc would be better and made the change.

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

Fixed.

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


Thanks and Regards,
Shlok Kyal

Attachment

v10-0001-Restrict-publishing-of-partitioned-table-with-fo.patch

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

From

Álvaro Herrera

Date:

28 March, 14:04:57

On 2025-Mar-28, Shlok Kyal wrote:

> On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > Also, surely we should document this restriction in the SGML docs
> > somewhere.
>
> 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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

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

From

Shlok Kyal

Date:

01 April, 17:48:21

On Fri, 28 Mar 2025 at 16:35, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-28, Shlok Kyal wrote:
>
> > On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > > Also, surely we should document this restriction in the SGML docs
> > > somewhere.
> >
> > 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.
>

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

Attachment

v11-0001-Restrict-publishing-of-partitioned-table-with-fo.patch

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

From

Sergey Tatarintsev

Date:

04 April, 08:06:49

01.04.2025 21:48, Shlok Kyal пишет:
> On Fri, 28 Mar 2025 at 16:35, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> On 2025-Mar-28, Shlok Kyal wrote:
>>
>>> On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>>> Also, surely we should document this restriction in the SGML docs
>>>> somewhere.
>>> 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.
>>
> 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

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

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

From

Shlok Kyal

Date:

04 April, 12:14:17

On Fri, 4 Apr 2025 at 10:36, Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> 01.04.2025 21:48, Shlok Kyal пишет:
> > On Fri, 28 Mar 2025 at 16:35, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >> On 2025-Mar-28, Shlok Kyal wrote:
> >>
> >>> On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >>>> Also, surely we should document this restriction in the SGML docs
> >>>> somewhere.
> >>> 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.
> >>
> > 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
>
> 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);
>          }
>
>          /*
>

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

Attachment

v12-0001-Restrict-publishing-of-partitioned-table-with-fo.patch