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:
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
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:
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)
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
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
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:
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
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
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:
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
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
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:
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
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Sergey Tatarintsev
Date:
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:
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
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
On 2025-Apr-01, Shlok Kyal wrote: > 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. Sadly I don't have time to describe the changes proposed here right now, but I'll do that early tomorrow. (Some minor changes are still needed, particularly the comments to publication_check_foreign_parts which are mostly unchanged from what your patch has. I'll do another review round tomorrow.) -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Attachment
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Sergey Tatarintsev
Date:
07.04.2025 03:27, Álvaro Herrera пишет:
On 2025-Apr-01, Shlok Kyal wrote: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.Sadly I don't have time to describe the changes proposed here right now, but I'll do that early tomorrow. (Some minor changes are still needed, particularly the comments to publication_check_foreign_parts which are mostly unchanged from what your patch has. I'll do another review round tomorrow.)
Hello!
I looked at the latest patch again and found one more place for list_free(). Also look at additional test case:
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index 51e463c112b..7fcc191feb9 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -442,6 +442,7 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, result = lappend_oid(result, partOid); } + list_free(all_parts); } else result = lappend_oid(result, relid); diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 49c9d98b668..e56aebc397a 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -1296,6 +1296,14 @@ SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3' -- foreign partition ALTER PUBLICATION pub1 SET (publish_via_partition_root); +CREATE SCHEMA sch5; +CREATE SCHEMA sch6; +CREATE TABLE sch6.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch5.part1 PARTITION OF sch6.tmain FOR VALUES FROM (0) TO (10) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch6.part2 PARTITION OF sch5.part1 FOR VALUES FROM (0) TO (5) SERVER fdw_server; +CREATE PUBLICATION pub4 FOR TABLES IN SCHEMA sch5 WITH (publish_via_partition_root); +SELECT pubname, tablename FROM pg_publication_tables WHERE pubname = 'pub4' ORDER BY pubname, tablename; + DROP PUBLICATION pub1; DROP PUBLICATION pub2; DROP PUBLICATION pub3;
I think this is a wrong assumption:
ScanKeyInit(&key[keycount++], Anum_pg_class_relispartition, BTEqualStrategyNumber, F_BOOLEQ, BoolGetDatum(false));
In this case sch5.part1 is partitioned table, but it also partition of table in different schema --
With best regards,
Sergey Tatarintsev,
PostgresPro
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
As promised, here's a rundown of the changes I did, mostly in order the patch shows them: - I reworded the documentation changes to read more coherent with the surrounding text. - It seemed wrong to have check_publication_add_relation() have the relation first as argument and publication later, so I turned them around. - I rewrote almost all of the error messages. The errdetails failed style (no initial capitals and no end period); one of the errmsg() was completely wrong (it said "cannot set parameter" instead of "cannot add table to publication"). I also moved the parameter name out of the translatable part of the message. - I removed the novel use of the term "foreign partition". We don't seem to use that anywhere that's user-visible, only in code comments. Instead I used "a partition that's a foreign table". I just did a new grep and I think I missed a couple, which I'm going to fix momentarily. - I renamed check_foreign_tables to publication_check_foreign_parts and moved to a different place in the file. The original name wasn't very good for an extern function. - I renamed check_partrel_has_foreign_table to RelationHasForeignPartition and moved it to partdesc.c. There's nothing that ties this function to pg_publication.c, so in my mind it made no sense for it to be there. I'm not 100% certain that partdesc.c is the best place for it, but if there's another, then I didn't see it. Also, I made it Assert() that it's not given a relation other than a partitioned table, which forces all callers to check the relkind before calling it. This is good because we don't need to open tables before ensuring the relkind is one that interests us. Also, I made it use the PartitionDesc->is_leaf flag array to determine whether to recurse or not; and use get_rel_relkind to get the relkind for leaf partitions, which is much cheaper than doing table_open. The original coding was kind of wasteful for no reason. - The check to prohibit CREATE FOREIGN TABLE as a partition in CreateForeignTable, was entirely in the wrong place. This is proven by the fact that the original coding had to do table_openrv() in order to know what the parent table was. This is terrible coding, because it means we resolve the table name to OID twice. (This is probably not so terrible because the first location that does it already acquires a good enough lock that it'd stay put; still, it's a bad coding pattern). I moved it to DefineRelation, in the block were we learn that we're creating a partition; it's easy to check there that the new relation is a foreign table. We don't even need to open the parent table at all, since it's already open. - In ATExecAttachPartition (and in the check that was in CreateForeignTable), there were a bunch of list_concat_unique_oids(). It's unlikely that this would bother anyone unless there are large numbers of publications, but the performance characteristics of doing this uniquification were unclear, and list_concat_unique() comments warn against it anyway. It seemed safer to just do list_concat() and a single sort/uniq step at the end, just before walking the list. - There were a bunch of list_frees() in there. I removed them all. They are pointless, unnecessary, and give a wrong sense of cleanliness. In reality, you will be leaking the second argument of each list_concat() anyway; to make this watertight, you'd have to read each of those lists into a variable, concat, then free the variable. The code would become much noisier coding. However, DDL code is normally leaky and that causes no major problems, because the memory context in use is going to be released or deleted soon afterwards. Freeing a few bytes ahead of time would only require more memory context bookkeeping to be executed, to very little gain. I think that's all I had ... -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
On 2025-Apr-07, Sergey Tatarintsev wrote: > I think this is a wrong assumption: > > ScanKeyInit(&key[keycount++], Anum_pg_class_relispartition, > BTEqualStrategyNumber, F_BOOLEQ, BoolGetDatum(false)); > > In this case sch5.part1 is partitioned table, but it also partition of table > in different schema Yeah, this looks ill-considered. After looking at this code some more, I'm withdrawing myself from here. (I don't mean just this patch in particular, I mean the logical replication code as a whole.) Thanks, -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La persona que no quería pecar / estaba obligada a sentarse en duras y empinadas sillas / desprovistas, por cierto de blandos atenuantes" (Patricio Vogel)
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
Here's the additional changes I made here before giving up on this. I think it needs some additional rethinking, not going to happen for 18. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "The Gord often wonders why people threaten never to come back after they've been told never to return" (www.actsofgord.com)
Attachment
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Shlok Kyal
Date:
On Mon, 7 Apr 2025 at 18:09, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Here's the additional changes I made here before giving up on this. > I think it needs some additional rethinking, not going to happen for 18. > Hi Alvaro, Thanks for reviewing the patch. The changes shared by you in [1], look good to me and I have added it to the latest patch. I have also included the changes shared by you in [2]. I have also addressed the issue reported by Sergey in [3]. Additionally, I made some changes to the comments, commit message and documentation to reflect the changes in the function name and to reduce the usage of 'foreign partition' term. I saw some comments by you in the patches: 1. + /* + * Keep lock till end of transaction: must prevent this table from + * being attached a foreign table until we're done. XXX does this + * prevent addition of a partition in a partitioned child? + */ This is same as the issue reported by Sergey in [3]. I have addressed the issue in the latest patch. So, I have modified this comment. 2. + * We also take a ShareLock on pg_partitioned_table to restrict addition + * of new partitioned table which may contain a foreign partition while + * publication is being created. XXX this is quite weird actually. This change was added to resolve the concurrency issue shared by Vignesh in [4]. I tried with different locks and found that lock with severity >= ShareLock was needed to avoid the concurrency issue. Initially I added ShareLock to pg_class, but to reduce the scope I added it to pg_partitioned_table instead. I cannot think of an alternate approach. Do you have any suggestions for this? [1]: https://www.postgresql.org/message-id/202504062027.tqmabk2h353o%40alvherre.pgsql [2]: https://www.postgresql.org/message-id/202504071239.kuj6m5a5wqxg%40alvherre.pgsql [3]: https://www.postgresql.org/message-id/c64352fa-9a30-4e0a-853a-a6b5b6d07f4e%40postgrespro.ru [4]: https://www.postgresql.org/message-id/CALDaNm2%2BeL22Sbvj74uS37xvt%3DhaQWcOwP15QnDuVeYsjHiffw%40mail.gmail.com Thanks and Regards, Shlok Kyal
Attachment
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Shlok Kyal
Date:
On Mon, 7 Apr 2025 at 09:43, Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> 07.04.2025 03:27, Álvaro Herrera пишет:
>
> On 2025-Apr-01, Shlok Kyal wrote:
>
> 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.
>
> Sadly I don't have time to describe the changes proposed here right now,
> but I'll do that early tomorrow. (Some minor changes are still needed,
> particularly the comments to publication_check_foreign_parts which are
> mostly unchanged from what your patch has. I'll do another review round
> tomorrow.)
>
> Hello!
>
> I looked at the latest patch again and found one more place for list_free(). Also look at additional test case:
>
> diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index
51e463c112b..7fcc191feb9100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@
-442,6+442,7 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,
result= lappend_oid(result, partOid); } + list_free(all_parts); } else
result = lappend_oid(result, relid); diff --git a/src/test/regress/sql/publication.sql
b/src/test/regress/sql/publication.sqlindex 49c9d98b668..e56aebc397a 100644 --- a/src/test/regress/sql/publication.sql
+++b/src/test/regress/sql/publication.sql @@ -1296,6 +1296,14 @@ SELECT pubname, tablename FROM pg_publication_tables
WHEREschemaname in ('sch3' -- foreign partition ALTER PUBLICATION pub1 SET (publish_via_partition_root); +CREATE
SCHEMAsch5; +CREATE SCHEMA sch6; +CREATE TABLE sch6.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch5.part1
PARTITIONOF sch6.tmain FOR VALUES FROM (0) TO (10) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch6.part2 PARTITION
OFsch5.part1 FOR VALUES FROM (0) TO (5) SERVER fdw_server; +CREATE PUBLICATION pub4 FOR TABLES IN SCHEMA sch5 WITH
(publish_via_partition_root);+SELECT pubname, tablename FROM pg_publication_tables WHERE pubname = 'pub4' ORDER BY
pubname,tablename; + DROP PUBLICATION pub1; DROP PUBLICATION pub2; DROP PUBLICATION pub3;
>
> I think this is a wrong assumption:
>
> ScanKeyInit(&key[keycount++], Anum_pg_class_relispartition, BTEqualStrategyNumber, F_BOOLEQ, BoolGetDatum(false));
>
> In this case sch5.part1 is partitioned table, but it also partition of table in different schema
>
Hi Sergey,
Thanks for reviewing the patch. I have addressed the comment in the
latest patch shared in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEW0QMiJXMqpPFRHni-q0Rm4R0hpZ0LdaqA%3DF3wvDUU6sQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
Re: Restrict publishing of partitioned table with a foreign table as partition
From
Álvaro Herrera
Date:
On 2025-Apr-28, Shlok Kyal wrote: > 2. > + * We also take a ShareLock on pg_partitioned_table to restrict addition > + * of new partitioned table which may contain a foreign partition while > + * publication is being created. XXX this is quite weird actually. > > This change was added to resolve the concurrency issue shared by > Vignesh in [4]. I tried with different locks and found that lock with > severity >= ShareLock was needed to avoid the concurrency issue. > Initially I added ShareLock to pg_class, but to reduce the scope I > added it to pg_partitioned_table instead. I cannot think of an > alternate approach. Do you have any suggestions for this? > [4]: https://www.postgresql.org/message-id/CALDaNm2%2BeL22Sbvj74uS37xvt%3DhaQWcOwP15QnDuVeYsjHiffw%40mail.gmail.com I think this is the sticky point in this patch. I think you need a clearer explanation (in code comments) of why you need this lock, and whether a weaker lock would be enough in some cases (see below); also I suspect that these locking considerations are going to be important for users so they're going to need to be documented in the SGML docs. What operations are blocked when you hold this lock? Is replication going to block altogether until the transaction that runs publication_check_foreign_parts() commits/aborts? This is important because it might mean that that users need to keep such transactions short. If your publication is FOR TABLES IN SCHEMA, can you do with blocking creation of partitions only in that schema, or only partitions of partitioned tables in that schema? Another point that just occurred to me is that pg_upgrade --check may need to alert users that if they have an incompatible setup in 18 or earlier, then an upgrade to 19 does not work until they have fixed the publications, detached the partitions, or some other remediation has been applied. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Linux transformó mi computadora, de una `máquina para hacer cosas', en un aparato realmente entretenido, sobre el cual cada día aprendo algo nuevo" (Jaime Salinas)