public inbox for [email protected]help / color / mirror / Atom feed
Re: Restrict publishing of partitioned table with a foreign table as partition 9+ messages / 4 participants [nested] [flat]
* Re: Restrict publishing of partitioned table with a foreign table as partition @ 2025-02-14 07:29 Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Shlok Kyal @ 2025-02-14 07:29 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Thu, 13 Feb 2025 at 20:12, vignesh C <[email protected]> wrote: > > On Thu, 13 Feb 2025 at 15:50, Shlok Kyal <[email protected]> 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 Attachments: [application/octet-stream] v7-0001-Restrict-publishing-of-partitioned-table-with-for.patch (25.0K, ../../CANhcyEWYJccdE0VP7dnMXGXvM3Lp5em_27hBu3tvi5N3=nWohg@mail.gmail.com/2-v7-0001-Restrict-publishing-of-partitioned-table-with-for.patch) download | inline diff: From a7528ba27c7dbd3159c94363c650f9674a63d5e1 Mon Sep 17 00:00:00 2001 From: Shlok Kyal <[email protected]> Date: Tue, 11 Feb 2025 15:32:55 +0530 Subject: [PATCH v7] Restrict publishing of partitioned table with foreign partition Logical replication of foreign table is not supported and we throw an error in this case. But when create a publication on a partitioned table that has a foreign partition, the initial sync of such table is successful and we should avoid such cases. Current Behaviour, when publication is created: 1. with publish_via_partition_root = true Root table is published and the initial data of foreign partitions are replicated. 2. with publish_via_partition_root = false and FOR ALL TABLES All leaf tables except foreign partitions are published. 3. with publish_via_partition_root = false and FOR TABLE/ FOR TABLES IN SCHEMA All leaf tables are published. Initial data of foreign partitions are replicated. With this patch we have following behaviour: 1. with publish_via_partition_root = true We throw an error when we try to publish a foreign partititon. Error is thrown when we try to create a publication on (or add to existing publication) a partitioned table with foreign partition, when try to create a foreign partition and when we try to attach foreign table (or a table with foreign partition) to existing published tables. 2. with publish_via_partition_root = false We skip publishing foreign partition which are part of the publication. This is done by avoid adding foreign partitions in pg_subscription_rel catalog table. --- src/backend/catalog/pg_publication.c | 146 +++++++++++++++++++--- src/backend/commands/foreigncmds.c | 47 +++++++ src/backend/commands/publicationcmds.c | 47 +++++++ src/backend/commands/tablecmds.c | 53 ++++++++ src/include/catalog/pg_publication.h | 5 + src/test/regress/expected/publication.out | 89 +++++++++++++ src/test/regress/sql/publication.sql | 81 ++++++++++++ 7 files changed, 451 insertions(+), 17 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index d6f94db5d9..2df2b1d94a 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -53,7 +53,7 @@ typedef struct * error if not. */ static void -check_publication_add_relation(Relation targetrel) +check_publication_add_relation(Relation targetrel, Publication *pub) { /* Must be a regular or partitioned table */ if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION && @@ -64,6 +64,19 @@ check_publication_add_relation(Relation targetrel) RelationGetRelationName(targetrel)), errdetail_relkind_not_supported(RelationGetForm(targetrel)->relkind))); + /* + * publish_via_root_partition cannot be true if it is a partitioned table + * and has any foreign partition + */ + if (pub->pubviaroot && + check_partrel_has_foreign_table(RelationGetForm(targetrel))) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot set parameter \"%s\" to true for publication \"%s\"", + "publish_via_partition_root", pub->name), + errdetail("partition table \"%s\" in publication contains a foreign partition", + RelationGetRelationName(targetrel)))); + /* Can't be system table */ if (IsCatalogRelation(targetrel)) ereport(ERROR, @@ -304,7 +317,7 @@ check_and_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt, /* * Gets the relations based on the publication partition option for a specified - * relation. + * relation. Foreign tables are not included. */ List * GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, @@ -313,25 +326,21 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE && pub_partopt != PUBLICATION_PART_ROOT) { - List *all_parts = find_all_inheritors(relid, NoLock, - NULL); + List *all_parts = find_all_inheritors(relid, NoLock, NULL); - if (pub_partopt == PUBLICATION_PART_ALL) - result = list_concat(result, all_parts); - else if (pub_partopt == PUBLICATION_PART_LEAF) + foreach_oid(partOid, all_parts) { - ListCell *lc; + char relkind = get_rel_relkind(partOid); - foreach(lc, all_parts) - { - Oid partOid = lfirst_oid(lc); + if (relkind == RELKIND_FOREIGN_TABLE) + continue; - if (get_rel_relkind(partOid) != RELKIND_PARTITIONED_TABLE) - result = lappend_oid(result, partOid); - } + if (pub_partopt == PUBLICATION_PART_LEAF && + relkind == RELKIND_PARTITIONED_TABLE) + continue; + + result = lappend_oid(result, partOid); } - else - Assert(false); } else result = lappend_oid(result, relid); @@ -463,7 +472,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, RelationGetRelationName(targetrel), pub->name))); } - check_publication_add_relation(targetrel); + check_publication_add_relation(targetrel, pub); /* Validate and translate column names into a Bitmapset of attnums. */ attnums = pub_collist_validate(pri->relation, pri->columns); @@ -703,6 +712,13 @@ publication_add_schema(Oid pubid, Oid schemaid, bool if_not_exists) check_publication_add_schema(schemaid); + /* + * If publish_via_partition_root is true, check if schema has any foreign + * partition + */ + if (pub->pubviaroot) + check_foreign_tables(schemaid, pub->name); + /* Form a tuple */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); @@ -1332,3 +1348,99 @@ pg_get_publication_tables(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } + +/* 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); + + foreach_oid(relid, relids) + { + Relation rel = table_open(relid, AccessShareLock); + + if (RelationGetForm(rel)->relkind == RELKIND_FOREIGN_TABLE) + has_foreign_tbl = true; + + table_close(rel, NoLock); + + if (has_foreign_tbl) + break; + } + } + + return has_foreign_tbl; +} + +/* + * If valid schemaid provided, check if the schema has a partition table which + * has a foreign partition. The partition tables in a schema can have partitions + * in other schema. We also need to check if such partitions are foreign + * partition. + * + * If valid schemaid is not provided, we get all partition tables and check if + * it has any foreign partition. We take a lock on partition tables so no new + * foreign partitions are added concurrently. + */ +void +check_foreign_tables(Oid schemaid, char *pubname) +{ + Relation classRel; + ScanKeyData key[2]; + int keycount = 0; + TableScanDesc scan; + HeapTuple tuple; + + classRel = table_open(RelationRelationId, AccessShareLock); + + ScanKeyInit(&key[keycount++], + Anum_pg_class_relkind, + BTEqualStrategyNumber, F_CHAREQ, + CharGetDatum(RELKIND_PARTITIONED_TABLE)); + + 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); + + if (check_partrel_has_foreign_table(relForm)) + { + List *ancestors = get_partition_ancestors(relForm->oid); + Oid parent_oid = relForm->oid; + char *parent_name; + + foreach_oid(ancestor, ancestors) + { + Oid ancestor_schemaid = get_rel_namespace(ancestor); + + if (ancestor_schemaid == schemaid) + parent_oid = ancestor; + } + + list_free(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/foreigncmds.c b/src/backend/commands/foreigncmds.c index c14e038d54..f41a24aea3 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -21,6 +21,7 @@ #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" +#include "catalog/partition.h" #include "catalog/pg_foreign_data_wrapper.h" #include "catalog/pg_foreign_server.h" #include "catalog/pg_foreign_table.h" @@ -1423,6 +1424,52 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid) ftrel = table_open(ForeignTableRelationId, RowExclusiveLock); + /* + * Check if it is a foreign partition and the partitioned table is not + * published or published with publish_via_partition_root option as false. + */ + if (stmt->base.partbound != NULL) + { + RangeVar *root = castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations))); + Relation rootrel = table_openrv(root, AccessShareLock); + + if (RelationGetForm(rootrel)->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid schemaid = RelationGetNamespace(rootrel); + List *puboids = GetRelationPublications(rootrel->rd_id); + List *ancestors; + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rootrel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot create table foreign partition \"%s\"", + get_rel_name(relid)), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + RelationGetRelationName(rootrel)))); + } + } + + table_close(rootrel, AccessShareLock); + } + /* * For now the owner cannot be specified on create. Use effective user ID. */ diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 150a768d16..e1121a94f9 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -876,6 +876,10 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) /* Associate objects with the publication. */ if (stmt->for_all_tables) { + /* Check if any partitioned table has foreign partition */ + if (publish_via_partition_root) + check_foreign_tables(InvalidOid, stmt->pubname); + /* Invalidate relcache so that publication info is rebuilt. */ CacheInvalidateRelcacheAll(); } @@ -1041,6 +1045,49 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, } } + /* + * If publish_via_partition_root is set to true, check if the publication + * already have any foreign partition + */ + if (publish_via_partition_root_given && publish_via_partition_root) + { + List *schemaoids = NIL; + List *relids = NIL; + + char *pubname = stmt->pubname; + + if (pubform->puballtables) + check_foreign_tables(InvalidOid, pubname); + + schemaoids = GetPublicationSchemas(pubform->oid); + + foreach_oid(schemaoid, schemaoids) + check_foreign_tables(schemaoid, pubname); + + relids = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT); + + foreach_oid(relid, relids) + { + HeapTuple reltup; + Form_pg_class relform; + + reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + relform = (Form_pg_class) GETSTRUCT(reltup); + + ReleaseSysCache(reltup); + + if (check_partrel_has_foreign_table(relform)) + { + 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", + get_rel_name(relid)))); + } + } + } + /* Everything ok, form a new tuple. */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 5823fce934..fccc1082f4 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -19440,6 +19440,59 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot attach temporary relation of another session as partition"))); + /* + * Check if attachrel is a foreign table or a partitioned table with + * foreign partition and rel is not part of publication with option + * publish_via_partition_root as true. + */ + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE || + check_partrel_has_foreign_table(RelationGetForm(attachrel))) + { + Oid schemaid = RelationGetNamespace(rel); + List *puboids = GetRelationPublications(rel->rd_id); + List *ancestors; + char *relname = get_rel_name(rel->rd_id); + char *attachrelname = get_rel_name(attachrel->rd_id); + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + { + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach foreign table \"%s\" to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + else + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach table \"%s\" with foreign partition to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + } + } + } + /* * Check if attachrel has any identity columns or any columns that aren't * in the parent. diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 48c7d1a861..50705d12c9 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -19,6 +19,7 @@ #include "catalog/genbki.h" #include "catalog/objectaddress.h" +#include "catalog/pg_class.h" #include "catalog/pg_publication_d.h" /* IWYU pragma: export */ /* ---------------- @@ -191,4 +192,8 @@ extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols, extern Bitmapset *pub_form_cols_map(Relation relation, PublishGencolsType include_gencols_type); +extern bool check_partrel_has_foreign_table(Form_pg_class relform); + +extern void check_foreign_tables(Oid schemaid, char *pubname); + #endif /* PG_PUBLICATION_H */ diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 4de96c04f9..6a0c245bd6 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -1924,6 +1924,95 @@ DROP PUBLICATION pub1; DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +ERROR: cannot attach table "part2" with foreign partition to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +DROP PUBLICATION pub1; +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + pubname | tablename +---------+----------- + pub1 | part1 + pub2 | part1 + pub3 | part1 +(3 rows) + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 68001de400..49c9d98b66 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -1223,6 +1223,87 @@ DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== + +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; + +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); + +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); + +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +DROP PUBLICATION pub1; + +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; + +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); + +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; + RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> @ 2025-02-17 14:42 ` vignesh C <[email protected]> 2025-02-18 10:29 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: vignesh C @ 2025-02-17 14:42 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> @ 2025-02-18 10:29 ` Shlok Kyal <[email protected]> 2025-02-20 15:48 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Shlok Kyal @ 2025-02-18 10:29 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Mon, 17 Feb 2025 at 20:13, vignesh C <[email protected]> wrote: > > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <[email protected]> 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. Thanks and Regards, Shlok Kyal Attachments: [application/octet-stream] v8-0001-Restrict-publishing-of-partitioned-table-with-for.patch (25.7K, ../../CANhcyEUt8SfTfWpRdZBBJ=WBxH=x2-VXk5BzdJvXi=UvdwUiSA@mail.gmail.com/2-v8-0001-Restrict-publishing-of-partitioned-table-with-for.patch) download | inline diff: From 56cf18870d442cc3c23173997dbca8a060c65f43 Mon Sep 17 00:00:00 2001 From: Shlok Kyal <[email protected]> Date: Tue, 11 Feb 2025 15:32:55 +0530 Subject: [PATCH v8] Restrict publishing of partitioned table with foreign partition Logical replication of foreign table is not supported and we throw an error in this case. But when create a publication on a partitioned table that has a foreign partition, the initial sync of such table is successful and we should avoid such cases. Current Behaviour, when publication is created: 1. with publish_via_partition_root = true Root table is published and the initial data of foreign partitions are replicated. 2. with publish_via_partition_root = false and FOR ALL TABLES All leaf tables except foreign partitions are published. 3. with publish_via_partition_root = false and FOR TABLE/ FOR TABLES IN SCHEMA All leaf tables are published. Initial data of foreign partitions are replicated. With this patch we have following behaviour: 1. with publish_via_partition_root = true We throw an error when we try to publish a foreign partititon. Error is thrown when we try to create a publication on (or add to existing publication) a partitioned table with foreign partition, when try to create a foreign partition and when we try to attach foreign table (or a table with foreign partition) to existing published tables. 2. with publish_via_partition_root = false We skip publishing foreign partition which are part of the publication. This is done by avoid adding foreign partitions in pg_subscription_rel catalog table. We have introduced two functions 'check_partrel_has_foreign_table' and 'check_foreign_tables'. In 'check_partrel_has_foreign_table' we go through the child nodes of a partition and check if it has a foreign table. While doing so, we take a AccessShareLock on each partition table to avoid creation of foreign partition concurrently. In 'check_foreign_tables' if schema id is provided we check for each partitioned table in that schema if it has a foreign partition, or if schema id is not provided we check for each partitioned table in the database if it has a foreign partition. While doing so we take a ShareLock on pg_class so no partition table is created concurrently after this check. --- src/backend/catalog/pg_publication.c | 137 +++++++++++++++++++--- src/backend/commands/foreigncmds.c | 47 ++++++++ src/backend/commands/publicationcmds.c | 47 ++++++++ src/backend/commands/tablecmds.c | 53 +++++++++ src/include/catalog/pg_publication.h | 5 + src/test/regress/expected/publication.out | 89 ++++++++++++++ src/test/regress/sql/publication.sql | 81 +++++++++++++ 7 files changed, 442 insertions(+), 17 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index d6f94db5d99..d45577679ec 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -53,7 +53,7 @@ typedef struct * error if not. */ static void -check_publication_add_relation(Relation targetrel) +check_publication_add_relation(Relation targetrel, Publication *pub) { /* Must be a regular or partitioned table */ if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION && @@ -64,6 +64,19 @@ check_publication_add_relation(Relation targetrel) RelationGetRelationName(targetrel)), errdetail_relkind_not_supported(RelationGetForm(targetrel)->relkind))); + /* + * publish_via_root_partition cannot be true if it is a partitioned table + * and has any foreign partition + */ + if (pub->pubviaroot && + check_partrel_has_foreign_table(RelationGetForm(targetrel))) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot set parameter \"%s\" to true for publication \"%s\"", + "publish_via_partition_root", pub->name), + errdetail("partition table \"%s\" in publication contains a foreign partition", + RelationGetRelationName(targetrel)))); + /* Can't be system table */ if (IsCatalogRelation(targetrel)) ereport(ERROR, @@ -304,7 +317,7 @@ check_and_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt, /* * Gets the relations based on the publication partition option for a specified - * relation. + * relation. Foreign tables are not included. */ List * GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, @@ -313,25 +326,21 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE && pub_partopt != PUBLICATION_PART_ROOT) { - List *all_parts = find_all_inheritors(relid, NoLock, - NULL); + List *all_parts = find_all_inheritors(relid, NoLock, NULL); - if (pub_partopt == PUBLICATION_PART_ALL) - result = list_concat(result, all_parts); - else if (pub_partopt == PUBLICATION_PART_LEAF) + foreach_oid(partOid, all_parts) { - ListCell *lc; + char relkind = get_rel_relkind(partOid); - foreach(lc, all_parts) - { - Oid partOid = lfirst_oid(lc); + if (relkind == RELKIND_FOREIGN_TABLE) + continue; - if (get_rel_relkind(partOid) != RELKIND_PARTITIONED_TABLE) - result = lappend_oid(result, partOid); - } + if (pub_partopt == PUBLICATION_PART_LEAF && + relkind == RELKIND_PARTITIONED_TABLE) + continue; + + result = lappend_oid(result, partOid); } - else - Assert(false); } else result = lappend_oid(result, relid); @@ -463,7 +472,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, RelationGetRelationName(targetrel), pub->name))); } - check_publication_add_relation(targetrel); + check_publication_add_relation(targetrel, pub); /* Validate and translate column names into a Bitmapset of attnums. */ attnums = pub_collist_validate(pri->relation, pri->columns); @@ -703,6 +712,13 @@ publication_add_schema(Oid pubid, Oid schemaid, bool if_not_exists) check_publication_add_schema(schemaid); + /* + * If publish_via_partition_root is true, check if schema has any foreign + * partition + */ + if (pub->pubviaroot) + check_foreign_tables(schemaid, pub->name); + /* Form a tuple */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); @@ -1332,3 +1348,90 @@ pg_get_publication_tables(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } + +/* 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 = 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, NoLock); + + if (has_foreign_tbl) + break; + } + } + + return has_foreign_tbl; +} + +/* + * If valid schemaid provided, check if the schema has a partition table which + * has a foreign partition. The partition tables in a schema can have partitions + * in other schema. We also need to check if such partitions are foreign + * partition. + * + * If valid schemaid is not provided, we get all partition tables and check if + * it has any foreign partition. We take a lock on partition tables so no new + * foreign partitions are added concurrently. + * + * We take a ShareLock on pg_class to restrict addition of new partitioned table + * which may contain a foreign partition while publication is being created. + */ +void +check_foreign_tables(Oid schemaid, char *pubname) +{ + Relation classRel; + ScanKeyData key[3]; + int keycount = 0; + TableScanDesc scan; + HeapTuple tuple; + + classRel = table_open(RelationRelationId, ShareLock); + + /* Get the root nodes of partitioned table */ + ScanKeyInit(&key[keycount++], + Anum_pg_class_relkind, + BTEqualStrategyNumber, F_CHAREQ, + CharGetDatum(RELKIND_PARTITIONED_TABLE)); + + ScanKeyInit(&key[keycount++], + Anum_pg_class_relispartition, + BTEqualStrategyNumber, F_BOOLEQ, + BoolGetDatum(false)); + + /* If schema id is provided check partitioned table in that schema */ + 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); + + if (check_partrel_has_foreign_table(relForm)) + 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", + get_rel_name(relForm->oid)))); + } + + table_endscan(scan); + table_close(classRel, NoLock); +} diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index c14e038d54f..f41a24aea39 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -21,6 +21,7 @@ #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" +#include "catalog/partition.h" #include "catalog/pg_foreign_data_wrapper.h" #include "catalog/pg_foreign_server.h" #include "catalog/pg_foreign_table.h" @@ -1423,6 +1424,52 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid) ftrel = table_open(ForeignTableRelationId, RowExclusiveLock); + /* + * Check if it is a foreign partition and the partitioned table is not + * published or published with publish_via_partition_root option as false. + */ + if (stmt->base.partbound != NULL) + { + RangeVar *root = castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations))); + Relation rootrel = table_openrv(root, AccessShareLock); + + if (RelationGetForm(rootrel)->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid schemaid = RelationGetNamespace(rootrel); + List *puboids = GetRelationPublications(rootrel->rd_id); + List *ancestors; + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rootrel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot create table foreign partition \"%s\"", + get_rel_name(relid)), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + RelationGetRelationName(rootrel)))); + } + } + + table_close(rootrel, AccessShareLock); + } + /* * For now the owner cannot be specified on create. Use effective user ID. */ diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 150a768d16f..e1121a94f92 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -876,6 +876,10 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) /* Associate objects with the publication. */ if (stmt->for_all_tables) { + /* Check if any partitioned table has foreign partition */ + if (publish_via_partition_root) + check_foreign_tables(InvalidOid, stmt->pubname); + /* Invalidate relcache so that publication info is rebuilt. */ CacheInvalidateRelcacheAll(); } @@ -1041,6 +1045,49 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, } } + /* + * If publish_via_partition_root is set to true, check if the publication + * already have any foreign partition + */ + if (publish_via_partition_root_given && publish_via_partition_root) + { + List *schemaoids = NIL; + List *relids = NIL; + + char *pubname = stmt->pubname; + + if (pubform->puballtables) + check_foreign_tables(InvalidOid, pubname); + + schemaoids = GetPublicationSchemas(pubform->oid); + + foreach_oid(schemaoid, schemaoids) + check_foreign_tables(schemaoid, pubname); + + relids = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT); + + foreach_oid(relid, relids) + { + HeapTuple reltup; + Form_pg_class relform; + + reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + relform = (Form_pg_class) GETSTRUCT(reltup); + + ReleaseSysCache(reltup); + + if (check_partrel_has_foreign_table(relform)) + { + 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", + get_rel_name(relid)))); + } + } + } + /* Everything ok, form a new tuple. */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 72a1b64c2a2..e96e58903ec 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -19440,6 +19440,59 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot attach temporary relation of another session as partition"))); + /* + * Check if attachrel is a foreign table or a partitioned table with + * foreign partition and rel is not part of publication with option + * publish_via_partition_root as true. + */ + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE || + check_partrel_has_foreign_table(RelationGetForm(attachrel))) + { + Oid schemaid = RelationGetNamespace(rel); + List *puboids = GetRelationPublications(rel->rd_id); + List *ancestors; + char *relname = get_rel_name(rel->rd_id); + char *attachrelname = get_rel_name(attachrel->rd_id); + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + { + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach foreign table \"%s\" to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + else + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach table \"%s\" with foreign partition to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + } + } + } + /* * Check if attachrel has any identity columns or any columns that aren't * in the parent. diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 48c7d1a8615..50705d12c98 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -19,6 +19,7 @@ #include "catalog/genbki.h" #include "catalog/objectaddress.h" +#include "catalog/pg_class.h" #include "catalog/pg_publication_d.h" /* IWYU pragma: export */ /* ---------------- @@ -191,4 +192,8 @@ extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols, extern Bitmapset *pub_form_cols_map(Relation relation, PublishGencolsType include_gencols_type); +extern bool check_partrel_has_foreign_table(Form_pg_class relform); + +extern void check_foreign_tables(Oid schemaid, char *pubname); + #endif /* PG_PUBLICATION_H */ diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 4de96c04f9d..6a0c245bd60 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -1924,6 +1924,95 @@ DROP PUBLICATION pub1; DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +ERROR: cannot attach table "part2" with foreign partition to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +DROP PUBLICATION pub1; +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + pubname | tablename +---------+----------- + pub1 | part1 + pub2 | part1 + pub3 | part1 +(3 rows) + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 68001de4000..49c9d98b668 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -1223,6 +1223,87 @@ DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== + +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; + +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); + +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); + +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +DROP PUBLICATION pub1; + +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; + +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); + +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; + RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-02-18 10:29 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> @ 2025-02-20 15:48 ` vignesh C <[email protected]> 2025-03-24 09:33 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: vignesh C @ 2025-02-20 15:48 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Tue, 18 Feb 2025 at 15:59, Shlok Kyal <[email protected]> wrote: > > On Mon, 17 Feb 2025 at 20:13, vignesh C <[email protected]> wrote: > > > > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <[email protected]> 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 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-02-18 10:29 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-20 15:48 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> @ 2025-03-24 09:33 ` Shlok Kyal <[email protected]> 2025-03-24 15:47 ` Re: Restrict publishing of partitioned table with a foreign table as partition Álvaro Herrera <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Shlok Kyal @ 2025-03-24 09:33 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Thu, 20 Feb 2025 at 21:18, vignesh C <[email protected]> wrote: > > On Tue, 18 Feb 2025 at 15:59, Shlok Kyal <[email protected]> wrote: > > > > On Mon, 17 Feb 2025 at 20:13, vignesh C <[email protected]> wrote: > > > > > > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal <[email protected]> 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 Attachments: [application/octet-stream] v9-0001-Restrict-publishing-of-partitioned-table-with-for.patch (26.1K, ../../CANhcyEXbKwQ4ySy0fSzX46N7sauJsvteWMTpYMci_Q03R_ewQA@mail.gmail.com/2-v9-0001-Restrict-publishing-of-partitioned-table-with-for.patch) download | inline diff: From bda55bfffff9f0487730f5ddda34bc0af635faf0 Mon Sep 17 00:00:00 2001 From: Shlok Kyal <[email protected]> Date: Tue, 11 Feb 2025 15:32:55 +0530 Subject: [PATCH v9] Restrict publishing of partitioned table with foreign partition Logical replication of foreign table is not supported and we throw an error in this case. But when create a publication on a partitioned table that has a foreign partition, the initial sync of such table is successful and we should avoid such cases. Current Behaviour in HEAD, when publication is created: 1. with publish_via_partition_root = true Root table is published and the initial data of foreign partitions are replicated. 2. with publish_via_partition_root = false and FOR ALL TABLES All leaf tables except foreign partitions are published. 3. with publish_via_partition_root = false and FOR TABLE/ FOR TABLES IN SCHEMA All leaf tables are published. Initial data of foreign partitions are replicated. With this patch we have following behaviour: 1. with publish_via_partition_root = true We throw an error when we try to publish a foreign partititon. Error is thrown when we try to create a publication on (or add to existing publication) a partitioned table with foreign partition, when try to create a foreign partition and when we try to attach foreign table (or a table with foreign partition) to existing published tables. 2. with publish_via_partition_root = false We skip publishing foreign partition which are part of the publication. This is done by avoid adding foreign partitions in pg_subscription_rel catalog table. We have introduced two functions 'check_partrel_has_foreign_table' and 'check_foreign_tables'. In 'check_partrel_has_foreign_table' we go through the child nodes of a partition and check if it has a foreign table. While doing so, we take a AccessShareLock on each partition table to avoid creation of foreign partition concurrently. In 'check_foreign_tables' if schema id is provided we check for each partitioned table in that schema if it has a foreign partition, or if schema id is not provided we check for each partitioned table in the database if it has a foreign partition. While doing so we take a ShareLock on pg_partitioned_table so no partition table is created concurrently after this check. --- src/backend/catalog/pg_publication.c | 141 +++++++++++++++++++--- src/backend/commands/foreigncmds.c | 47 ++++++++ src/backend/commands/publicationcmds.c | 47 ++++++++ src/backend/commands/tablecmds.c | 53 ++++++++ src/include/catalog/pg_publication.h | 5 + src/test/regress/expected/publication.out | 89 ++++++++++++++ src/test/regress/sql/publication.sql | 81 +++++++++++++ 7 files changed, 446 insertions(+), 17 deletions(-) diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index d6f94db5d99..d3d8173509e 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -26,6 +26,7 @@ #include "catalog/partition.h" #include "catalog/pg_inherits.h" #include "catalog/pg_namespace.h" +#include "catalog/pg_partitioned_table.h" #include "catalog/pg_publication.h" #include "catalog/pg_publication_namespace.h" #include "catalog/pg_publication_rel.h" @@ -53,7 +54,7 @@ typedef struct * error if not. */ static void -check_publication_add_relation(Relation targetrel) +check_publication_add_relation(Relation targetrel, Publication *pub) { /* Must be a regular or partitioned table */ if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION && @@ -64,6 +65,19 @@ check_publication_add_relation(Relation targetrel) RelationGetRelationName(targetrel)), errdetail_relkind_not_supported(RelationGetForm(targetrel)->relkind))); + /* + * publish_via_root_partition cannot be true if it is a partitioned table + * and has any foreign partition + */ + if (pub->pubviaroot && + check_partrel_has_foreign_table(RelationGetForm(targetrel))) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot set parameter \"%s\" to true for publication \"%s\"", + "publish_via_partition_root", pub->name), + errdetail("partition table \"%s\" in publication contains a foreign partition", + RelationGetRelationName(targetrel)))); + /* Can't be system table */ if (IsCatalogRelation(targetrel)) ereport(ERROR, @@ -304,7 +318,7 @@ check_and_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt, /* * Gets the relations based on the publication partition option for a specified - * relation. + * relation. Foreign tables are not included. */ List * GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, @@ -313,25 +327,21 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE && pub_partopt != PUBLICATION_PART_ROOT) { - List *all_parts = find_all_inheritors(relid, NoLock, - NULL); + List *all_parts = find_all_inheritors(relid, NoLock, NULL); - if (pub_partopt == PUBLICATION_PART_ALL) - result = list_concat(result, all_parts); - else if (pub_partopt == PUBLICATION_PART_LEAF) + foreach_oid(partOid, all_parts) { - ListCell *lc; + char relkind = get_rel_relkind(partOid); - foreach(lc, all_parts) - { - Oid partOid = lfirst_oid(lc); + if (relkind == RELKIND_FOREIGN_TABLE) + continue; - if (get_rel_relkind(partOid) != RELKIND_PARTITIONED_TABLE) - result = lappend_oid(result, partOid); - } + if (pub_partopt == PUBLICATION_PART_LEAF && + relkind == RELKIND_PARTITIONED_TABLE) + continue; + + result = lappend_oid(result, partOid); } - else - Assert(false); } else result = lappend_oid(result, relid); @@ -463,7 +473,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, RelationGetRelationName(targetrel), pub->name))); } - check_publication_add_relation(targetrel); + check_publication_add_relation(targetrel, pub); /* Validate and translate column names into a Bitmapset of attnums. */ attnums = pub_collist_validate(pri->relation, pri->columns); @@ -703,6 +713,13 @@ publication_add_schema(Oid pubid, Oid schemaid, bool if_not_exists) check_publication_add_schema(schemaid); + /* + * If publish_via_partition_root is true, check if schema has any foreign + * partition + */ + if (pub->pubviaroot) + check_foreign_tables(schemaid, pub->name); + /* Form a tuple */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); @@ -1332,3 +1349,93 @@ pg_get_publication_tables(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } + +/* 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 = 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, NoLock); + + if (has_foreign_tbl) + break; + } + } + + return has_foreign_tbl; +} + +/* + * If valid schemaid provided, check if the schema has a partition table which + * has a foreign partition. The partition tables in a schema can have partitions + * in other schema. We also need to check if such partitions are foreign + * partition. + * + * If valid schemaid is not provided, we get all partition tables and check if + * it has any foreign partition. We take a lock on partition tables so no new + * foreign partitions are added concurrently. + * + * We take a ShareLock on pg_class to restrict addition of new partitioned table + * which may contain a foreign partition while publication is being created. + */ +void +check_foreign_tables(Oid schemaid, char *pubname) +{ + Relation classRel; + Relation partRel; + ScanKeyData key[3]; + int keycount = 0; + TableScanDesc scan; + HeapTuple tuple; + + classRel = table_open(RelationRelationId, AccessShareLock); + partRel = table_open(PartitionedRelationId, ShareLock); + + /* Get the root nodes of partitioned table */ + ScanKeyInit(&key[keycount++], + Anum_pg_class_relkind, + BTEqualStrategyNumber, F_CHAREQ, + CharGetDatum(RELKIND_PARTITIONED_TABLE)); + + ScanKeyInit(&key[keycount++], + Anum_pg_class_relispartition, + BTEqualStrategyNumber, F_BOOLEQ, + BoolGetDatum(false)); + + /* If schema id is provided check partitioned table in that schema */ + 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); + + if (check_partrel_has_foreign_table(relForm)) + 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", + get_rel_name(relForm->oid)))); + } + + table_endscan(scan); + table_close(classRel, AccessShareLock); + table_close(partRel, NoLock); +} diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index c14e038d54f..f41a24aea39 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -21,6 +21,7 @@ #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" +#include "catalog/partition.h" #include "catalog/pg_foreign_data_wrapper.h" #include "catalog/pg_foreign_server.h" #include "catalog/pg_foreign_table.h" @@ -1423,6 +1424,52 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid) ftrel = table_open(ForeignTableRelationId, RowExclusiveLock); + /* + * Check if it is a foreign partition and the partitioned table is not + * published or published with publish_via_partition_root option as false. + */ + if (stmt->base.partbound != NULL) + { + RangeVar *root = castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations))); + Relation rootrel = table_openrv(root, AccessShareLock); + + if (RelationGetForm(rootrel)->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid schemaid = RelationGetNamespace(rootrel); + List *puboids = GetRelationPublications(rootrel->rd_id); + List *ancestors; + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rootrel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot create table foreign partition \"%s\"", + get_rel_name(relid)), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + RelationGetRelationName(rootrel)))); + } + } + + table_close(rootrel, AccessShareLock); + } + /* * For now the owner cannot be specified on create. Use effective user ID. */ diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 0b23d94c38e..f74cbb2abb3 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -915,6 +915,10 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) /* Associate objects with the publication. */ if (stmt->for_all_tables) { + /* Check if any partitioned table has foreign partition */ + if (publish_via_partition_root) + check_foreign_tables(InvalidOid, stmt->pubname); + /* Invalidate relcache so that publication info is rebuilt. */ CacheInvalidateRelcacheAll(); } @@ -1080,6 +1084,49 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, } } + /* + * If publish_via_partition_root is set to true, check if the publication + * already have any foreign partition + */ + if (publish_via_partition_root_given && publish_via_partition_root) + { + List *schemaoids = NIL; + List *relids = NIL; + + char *pubname = stmt->pubname; + + if (pubform->puballtables) + check_foreign_tables(InvalidOid, pubname); + + schemaoids = GetPublicationSchemas(pubform->oid); + + foreach_oid(schemaoid, schemaoids) + check_foreign_tables(schemaoid, pubname); + + relids = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT); + + foreach_oid(relid, relids) + { + HeapTuple reltup; + Form_pg_class relform; + + reltup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + relform = (Form_pg_class) GETSTRUCT(reltup); + + ReleaseSysCache(reltup); + + if (check_partrel_has_foreign_table(relform)) + { + 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", + get_rel_name(relid)))); + } + } + } + /* Everything ok, form a new tuple. */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 1202544ebd0..dac1db2ce50 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -19644,6 +19644,59 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot attach temporary relation of another session as partition"))); + /* + * Check if attachrel is a foreign table or a partitioned table with + * foreign partition and rel is not part of publication with option + * publish_via_partition_root as true. + */ + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE || + check_partrel_has_foreign_table(RelationGetForm(attachrel))) + { + Oid schemaid = RelationGetNamespace(rel); + List *puboids = GetRelationPublications(rel->rd_id); + List *ancestors; + char *relname = get_rel_name(rel->rd_id); + char *attachrelname = get_rel_name(attachrel->rd_id); + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + { + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach foreign table \"%s\" to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + else + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach table \"%s\" with foreign partition to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + } + } + } + /* * Check if attachrel has any identity columns or any columns that aren't * in the parent. diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 48c7d1a8615..50705d12c98 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -19,6 +19,7 @@ #include "catalog/genbki.h" #include "catalog/objectaddress.h" +#include "catalog/pg_class.h" #include "catalog/pg_publication_d.h" /* IWYU pragma: export */ /* ---------------- @@ -191,4 +192,8 @@ extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols, extern Bitmapset *pub_form_cols_map(Relation relation, PublishGencolsType include_gencols_type); +extern bool check_partrel_has_foreign_table(Form_pg_class relform); + +extern void check_foreign_tables(Oid schemaid, char *pubname); + #endif /* PG_PUBLICATION_H */ diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 4de96c04f9d..6a0c245bd60 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -1924,6 +1924,95 @@ DROP PUBLICATION pub1; DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +ERROR: cannot attach table "part2" with foreign partition to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +DROP PUBLICATION pub1; +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + pubname | tablename +---------+----------- + pub1 | part1 + pub2 | part1 + pub3 | part1 +(3 rows) + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 68001de4000..49c9d98b668 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -1223,6 +1223,87 @@ DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== + +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; + +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); + +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); + +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +DROP PUBLICATION pub1; + +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; + +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); + +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; + RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-02-18 10:29 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-20 15:48 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-03-24 09:33 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> @ 2025-03-24 15:47 ` Álvaro Herrera <[email protected]> 2025-03-28 09:43 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Álvaro Herrera @ 2025-03-24 15:47 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; +Cc: vignesh C <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] 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) ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-02-18 10:29 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-20 15:48 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-03-24 09:33 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-03-24 15:47 ` Re: Restrict publishing of partitioned table with a foreign table as partition Álvaro Herrera <[email protected]> @ 2025-03-28 09:43 ` Shlok Kyal <[email protected]> 2025-03-28 11:04 ` Re: Restrict publishing of partitioned table with a foreign table as partition Álvaro Herrera <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Shlok Kyal @ 2025-03-28 09:43 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: vignesh C <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <[email protected]> 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 Attachments: [application/octet-stream] v10-0001-Restrict-publishing-of-partitioned-table-with-fo.patch (28.3K, ../../CANhcyEU5R4qdkO8SLSswRWgBRDPEaapAQ61rCjjDBg4RNmmsrw@mail.gmail.com/2-v10-0001-Restrict-publishing-of-partitioned-table-with-fo.patch) download | inline diff: From f8b13f85faae6b8bd4594ac84504ac8fa0cd77d0 Mon Sep 17 00:00:00 2001 From: Shlok Kyal <[email protected]> Date: Fri, 28 Mar 2025 11:15:09 +0530 Subject: [PATCH v10] Restrict publishing of partitioned table with foreign partition Logical replication of foreign table is not supported and we throw an error in this case. But when create a publication on a partitioned table that has a foreign partition, the initial sync of such table is successful and we should avoid such cases. Current Behaviour in HEAD, when publication is created: 1. with publish_via_partition_root = true Root table is published and the initial data of foreign partitions are replicated. 2. with publish_via_partition_root = false and FOR ALL TABLES All leaf tables except foreign partitions are published. 3. with publish_via_partition_root = false and FOR TABLE/ FOR TABLES IN SCHEMA All leaf tables are published. Initial data of foreign partitions are replicated. With this patch we have following behaviour: 1. with publish_via_partition_root = true We throw an error when we try to publish a foreign partititon. Error is thrown when we try to create a publication on (or add to existing publication) a partitioned table with foreign partition, when try to create a foreign partition and when we try to attach foreign table (or a table with foreign partition) to existing published tables. 2. with publish_via_partition_root = false We skip publishing foreign partition which are part of the publication. This is done by avoid adding foreign partitions in pg_subscription_rel catalog table. We have introduced two functions 'check_partrel_has_foreign_table' and 'check_foreign_tables'. In 'check_partrel_has_foreign_table' we go through the child nodes of a partition and check if it has a foreign table. While doing so, we take a AccessShareLock on each partition table to avoid creation of foreign partition concurrently. In 'check_foreign_tables' if schema id is provided we check for each partitioned table in that schema if it has a foreign partition, or if schema id is not provided we check for each partitioned table in the database if it has a foreign partition. While doing so we take a ShareLock on pg_partitioned_table so no partition table is created concurrently after this check. --- doc/src/sgml/ref/create_publication.sgml | 5 + src/backend/catalog/pg_publication.c | 154 +++++++++++++++++++--- src/backend/commands/foreigncmds.c | 48 +++++++ src/backend/commands/publicationcmds.c | 43 ++++++ src/backend/commands/tablecmds.c | 57 ++++++++ src/include/catalog/pg_publication.h | 5 + src/test/regress/expected/publication.out | 89 +++++++++++++ src/test/regress/sql/publication.sql | 81 ++++++++++++ 8 files changed, 465 insertions(+), 17 deletions(-) diff --git a/doc/src/sgml/ref/create_publication.sgml b/doc/src/sgml/ref/create_publication.sgml index 73f0c8d89fb..c4e993b1c1a 100644 --- a/doc/src/sgml/ref/create_publication.sgml +++ b/doc/src/sgml/ref/create_publication.sgml @@ -257,6 +257,11 @@ CREATE PUBLICATION <replaceable class="parameter">name</replaceable> If this is enabled, <literal>TRUNCATE</literal> operations performed directly on partitions are not replicated. </para> + + <para> + If this is enabled, we cannot include a foreign table or a partitioned + table with a foreign partition in the publication. + </para> </listitem> </varlistentry> </variablelist></para> diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c index d6f94db5d99..0daa5be3469 100644 --- a/src/backend/catalog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -26,12 +26,14 @@ #include "catalog/partition.h" #include "catalog/pg_inherits.h" #include "catalog/pg_namespace.h" +#include "catalog/pg_partitioned_table.h" #include "catalog/pg_publication.h" #include "catalog/pg_publication_namespace.h" #include "catalog/pg_publication_rel.h" #include "catalog/pg_type.h" #include "commands/publicationcmds.h" #include "funcapi.h" +#include "partitioning/partdesc.h" #include "utils/array.h" #include "utils/builtins.h" #include "utils/catcache.h" @@ -53,7 +55,7 @@ typedef struct * error if not. */ static void -check_publication_add_relation(Relation targetrel) +check_publication_add_relation(Relation targetrel, Publication *pub) { /* Must be a regular or partitioned table */ if (RelationGetForm(targetrel)->relkind != RELKIND_RELATION && @@ -64,6 +66,18 @@ check_publication_add_relation(Relation targetrel) RelationGetRelationName(targetrel)), errdetail_relkind_not_supported(RelationGetForm(targetrel)->relkind))); + /* + * publish_via_root_partition cannot be true if it is a partitioned table + * and has any foreign partition. See check_foreign_tables for details. + */ + if (pub->pubviaroot && check_partrel_has_foreign_table(targetrel)) + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("cannot set parameter \"%s\" to true for publication \"%s\"", + "publish_via_partition_root", pub->name), + errdetail("partition table \"%s\" in publication contains a foreign partition", + RelationGetRelationName(targetrel)))); + /* Can't be system table */ if (IsCatalogRelation(targetrel)) ereport(ERROR, @@ -304,7 +318,7 @@ check_and_fetch_column_list(Publication *pub, Oid relid, MemoryContext mcxt, /* * Gets the relations based on the publication partition option for a specified - * relation. + * relation. Foreign tables are not included. */ List * GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, @@ -313,25 +327,21 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt, if (get_rel_relkind(relid) == RELKIND_PARTITIONED_TABLE && pub_partopt != PUBLICATION_PART_ROOT) { - List *all_parts = find_all_inheritors(relid, NoLock, - NULL); + List *all_parts = find_all_inheritors(relid, NoLock, NULL); - if (pub_partopt == PUBLICATION_PART_ALL) - result = list_concat(result, all_parts); - else if (pub_partopt == PUBLICATION_PART_LEAF) + foreach_oid(partOid, all_parts) { - ListCell *lc; + char relkind = get_rel_relkind(partOid); - foreach(lc, all_parts) - { - Oid partOid = lfirst_oid(lc); + if (relkind == RELKIND_FOREIGN_TABLE) + continue; - if (get_rel_relkind(partOid) != RELKIND_PARTITIONED_TABLE) - result = lappend_oid(result, partOid); - } + if (pub_partopt == PUBLICATION_PART_LEAF && + relkind == RELKIND_PARTITIONED_TABLE) + continue; + + result = lappend_oid(result, partOid); } - else - Assert(false); } else result = lappend_oid(result, relid); @@ -463,7 +473,7 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri, RelationGetRelationName(targetrel), pub->name))); } - check_publication_add_relation(targetrel); + check_publication_add_relation(targetrel, pub); /* Validate and translate column names into a Bitmapset of attnums. */ attnums = pub_collist_validate(pri->relation, pri->columns); @@ -703,6 +713,13 @@ publication_add_schema(Oid pubid, Oid schemaid, bool if_not_exists) check_publication_add_schema(schemaid); + /* + * If publish_via_partition_root is true, check if schema has any foreign + * partition + */ + if (pub->pubviaroot) + check_foreign_tables(schemaid, pub->name); + /* Form a tuple */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); @@ -1332,3 +1349,106 @@ pg_get_publication_tables(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } + +/* Check if a partitioned table has a foreign partition */ +bool +check_partrel_has_foreign_table(Relation rel) +{ + bool has_foreign_tbl = false; + + if (RelationGetForm(rel)->relkind == RELKIND_PARTITIONED_TABLE) + { + PartitionDesc pd = RelationGetPartitionDesc(rel, true); + + for (int i = 0; i < pd->nparts; i++) + { + Relation childrel = table_open(pd->oids[i], AccessShareLock); + + if (RelationGetForm(childrel)->relkind == RELKIND_FOREIGN_TABLE) + has_foreign_tbl = true; + else + has_foreign_tbl = check_partrel_has_foreign_table(childrel); + + table_close(childrel, NoLock); + + if (has_foreign_tbl) + break; + } + } + + return has_foreign_tbl; +} + +/* + * Protect against including foreign tables that are partitions of partitioned + * tables published by the given publication when publish_via_root_partition is + * true. This will not work correctly as the initial data from the foreign + * table can be replicated by the tablesync worker even though replication of + * foreign table is not supported. So we disallow the case here and in all DDL + * commands that would end up creating such a case indirectly. + * + * If valid schemaid provided, check if the schema has a partition table which + * has a foreign partition. The partition tables in a schema can have partitions + * in other schema. We also need to check if such partitions are foreign + * partition. + * + * If valid schemaid is not provided, we get all partition tables and check if + * it has any foreign partition. We take a lock on partition tables so no new + * foreign partitions are added concurrently. + * + * We 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. + */ +void +check_foreign_tables(Oid schemaid, char *pubname) +{ + Relation classRel; + Relation partRel; + ScanKeyData key[3]; + int keycount = 0; + TableScanDesc scan; + HeapTuple tuple; + + classRel = table_open(RelationRelationId, AccessShareLock); + partRel = table_open(PartitionedRelationId, ShareLock); + + /* Get the root nodes of partitioned table */ + ScanKeyInit(&key[keycount++], + Anum_pg_class_relkind, + BTEqualStrategyNumber, F_CHAREQ, + CharGetDatum(RELKIND_PARTITIONED_TABLE)); + + ScanKeyInit(&key[keycount++], + Anum_pg_class_relispartition, + BTEqualStrategyNumber, F_BOOLEQ, + BoolGetDatum(false)); + + /* If schema id is provided check partitioned table in that schema */ + 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); + Relation pubrel = table_open(relForm->oid, AccessShareLock); + + if (check_partrel_has_foreign_table(pubrel)) + 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", + get_rel_name(relForm->oid)))); + + table_close(pubrel, NoLock); + } + + table_endscan(scan); + table_close(classRel, AccessShareLock); + table_close(partRel, NoLock); +} diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index c14e038d54f..abc23baa59e 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -21,6 +21,7 @@ #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" +#include "catalog/partition.h" #include "catalog/pg_foreign_data_wrapper.h" #include "catalog/pg_foreign_server.h" #include "catalog/pg_foreign_table.h" @@ -1423,6 +1424,53 @@ CreateForeignTable(CreateForeignTableStmt *stmt, Oid relid) ftrel = table_open(ForeignTableRelationId, RowExclusiveLock); + /* + * Check if it is a foreign partition and the partitioned table is not + * published or published with publish_via_partition_root option as false. + * See check_foreign_tables for details. + */ + if (stmt->base.partbound != NULL) + { + RangeVar *root = linitial_node(RangeVar, stmt->base.inhRelations); + Relation rootrel = table_openrv(root, AccessShareLock); + + if (RelationGetForm(rootrel)->relkind == RELKIND_PARTITIONED_TABLE) + { + Oid schemaid = RelationGetNamespace(rootrel); + List *puboids = GetRelationPublications(rootrel->rd_id); + List *ancestors; + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rootrel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot create table foreign partition \"%s\"", + get_rel_name(relid)), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + RelationGetRelationName(rootrel)))); + } + } + + table_close(rootrel, AccessShareLock); + } + /* * For now the owner cannot be specified on create. Use effective user ID. */ diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 0b23d94c38e..e5fa1bf5b09 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -915,6 +915,10 @@ CreatePublication(ParseState *pstate, CreatePublicationStmt *stmt) /* Associate objects with the publication. */ if (stmt->for_all_tables) { + /* Check if any partitioned table has foreign partition */ + if (publish_via_partition_root) + check_foreign_tables(InvalidOid, stmt->pubname); + /* Invalidate relcache so that publication info is rebuilt. */ CacheInvalidateRelcacheAll(); } @@ -1080,6 +1084,45 @@ AlterPublicationOptions(ParseState *pstate, AlterPublicationStmt *stmt, } } + /* + * If publish_via_partition_root is set to true, check if the publication + * already have any foreign partition. See check_foreign_tables for details. + */ + if (publish_via_partition_root_given && publish_via_partition_root) + { + List *schemaoids = NIL; + List *relids = NIL; + + char *pubname = stmt->pubname; + + if (pubform->puballtables) + check_foreign_tables(InvalidOid, pubname); + + schemaoids = GetPublicationSchemas(pubform->oid); + + foreach_oid(schemaoid, schemaoids) + check_foreign_tables(schemaoid, pubname); + + relids = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT); + + foreach_oid(relid, relids) + { + Relation pubrel = table_open(relid, AccessShareLock); + + if (check_partrel_has_foreign_table(pubrel)) + { + 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", + get_rel_name(relid)))); + } + + table_close(pubrel, NoLock); + } + } + /* Everything ok, form a new tuple. */ memset(values, 0, sizeof(values)); memset(nulls, false, sizeof(nulls)); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index afb25007613..fc4db92bde7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -19570,6 +19570,10 @@ QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, * ALTER TABLE <name> ATTACH PARTITION <partition-name> FOR VALUES * * Return the address of the newly attached partition. + * + * If the would-be partition is a foreign table or a partitioned table with + * foreign partition, verify that the partitioned table is not in a publication + * with publish_via_partition_root=true. See check_foreign_tables for details. */ static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd, @@ -19714,6 +19718,59 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd, (errcode(ERRCODE_WRONG_OBJECT_TYPE), errmsg("cannot attach temporary relation of another session as partition"))); + /* + * Check if attachrel is a foreign table or a partitioned table with + * foreign partition and rel is not part of publication with option + * publish_via_partition_root as true. + */ + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE || + check_partrel_has_foreign_table(attachrel)) + { + Oid schemaid = RelationGetNamespace(rel); + List *puboids = GetRelationPublications(rel->rd_id); + List *ancestors; + char *relname = get_rel_name(rel->rd_id); + char *attachrelname = get_rel_name(attachrel->rd_id); + + puboids = list_concat_unique_oid(puboids, GetAllTablesPublications()); + puboids = list_concat_unique_oid(puboids, GetSchemaPublications(schemaid)); + ancestors = get_partition_ancestors(rel->rd_id); + + foreach_oid(ancestor, ancestors) + { + puboids = list_concat_unique_oid(puboids, + GetRelationPublications(ancestor)); + schemaid = get_rel_namespace(ancestor); + puboids = list_concat_unique_oid(puboids, + GetSchemaPublications(schemaid)); + } + + list_free(ancestors); + + foreach_oid(puboid, puboids) + { + Publication *pub = GetPublication(puboid); + + if (pub->pubviaroot) + { + if (attachrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach foreign table \"%s\" to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + else + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("cannot attach table \"%s\" with foreign partition to partition table \"%s\"", + attachrelname, relname), + errdetail("partition table \"%s\" is published with option publish_via_partition_root", + relname))); + } + } + } + /* * Check if attachrel has any identity columns or any columns that aren't * in the parent. diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 48c7d1a8615..e2919da2541 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -19,6 +19,7 @@ #include "catalog/genbki.h" #include "catalog/objectaddress.h" +#include "catalog/pg_class.h" #include "catalog/pg_publication_d.h" /* IWYU pragma: export */ /* ---------------- @@ -191,4 +192,8 @@ extern Bitmapset *pub_collist_to_bitmapset(Bitmapset *columns, Datum pubcols, extern Bitmapset *pub_form_cols_map(Relation relation, PublishGencolsType include_gencols_type); +extern bool check_partrel_has_foreign_table(Relation rel); + +extern void check_foreign_tables(Oid schemaid, char *pubname); + #endif /* PG_PUBLICATION_H */ diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 4de96c04f9d..6a0c245bd60 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -1924,6 +1924,95 @@ DROP PUBLICATION pub1; DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); +ERROR: cannot attach table "part2" with foreign partition to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +ERROR: cannot create table foreign partition "part3_1" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +ERROR: cannot attach foreign table "part3_2" to partition table "tmain" +DETAIL: partition table "tmain" is published with option publish_via_partition_root +DROP PUBLICATION pub1; +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + pubname | tablename +---------+----------- + pub1 | part1 + pub2 | part1 + pub3 | part1 +(3 rows) + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); +ERROR: cannot set parameter "publish_via_partition_root" to true for publication "pub1" +DETAIL: partition table "tmain" in publication contains a foreign partition +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 68001de4000..49c9d98b668 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -1223,6 +1223,87 @@ DROP PUBLICATION pub2; DROP TABLE gencols; RESET client_min_messages; +-- ====================================================== + +-- Test when foreign table is a partition of a partitioned table on which +-- publication is created +SET client_min_messages = 'ERROR'; +CREATE FOREIGN DATA WRAPPER test_fdw; +CREATE SERVER fdw_server FOREIGN DATA WRAPPER test_fdw; + +CREATE SCHEMA sch3; +CREATE TABLE sch3.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch3.part1 PARTITION OF sch3.tmain FOR VALUES FROM (0) TO (5); +CREATE TABLE sch3.part2(id int) PARTITION BY RANGE(id); +CREATE FOREIGN TABLE sch3.part2_1 PARTITION OF sch3.part2 FOR VALUES FROM (5) TO (10) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create publications with publish_via_partition_root = true, if table +-- has a foreign partition +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch3 WITH (publish_via_partition_root); +CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_via_partition_root); + +-- Test when a partitioned table with foreign table as a partition is attached +-- to partitioned table which is already published +ALTER TABLE sch3.tmain DETACH PARTITION sch3.part2; +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain WITH (publish_via_partition_root); +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part2 FOR VALUES FROM (5) TO (10); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_2(id int) SERVER fdw_server; +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +CREATE SCHEMA sch4; +CREATE TABLE sch4.tmain(id int) PARTITION BY RANGE(id); + +-- publication created with FOR TABLES IN SCHEMA +DROP PUBLICATION pub1; +CREATE PUBLICATION pub1 FOR TABLES IN SCHEMA sch4 WITH (publish_via_partition_root); + +-- Can't create foreign partition of published table with +-- publish_via_partition_root = true +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch4.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Can't attach foreign partition to published table with +-- publish_via_partition_root = true +ALTER TABLE sch4.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); +DROP PUBLICATION pub1; + +-- Test with publish_via_partition_root = false +-- Foreign partitions are skipped by default +CREATE PUBLICATION pub1 FOR TABLE sch3.tmain; +CREATE PUBLICATION pub2 FOR TABLES IN SCHEMA sch3; +CREATE PUBLICATION pub3 FOR ALL TABLES; + +-- Create foreign partition of published table with +-- publish_via_partition_root = false +CREATE FOREIGN TABLE sch3.part3_1 PARTITION OF sch3.tmain FOR VALUES FROM (10) TO (15) SERVER fdw_server; + +-- Attach foreign partition to published table +-- publish_via_partition_root = false +ALTER TABLE sch3.tmain ATTACH PARTITION sch3.part3_2 FOR VALUES FROM (15) TO (20); + +-- Check the published tables +SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3', 'sch4') ORDER BY pubname, tablename; + +-- Can't alter publish_via_partition_root to true, if publication already have +-- foreign partition +ALTER PUBLICATION pub1 SET (publish_via_partition_root); + +DROP PUBLICATION pub1; +DROP PUBLICATION pub2; +DROP PUBLICATION pub3; +DROP SCHEMA sch3 CASCADE; +DROP SCHEMA sch4 CASCADE; +DROP SERVER fdw_server; +DROP FOREIGN DATA WRAPPER test_fdw; + RESET SESSION AUTHORIZATION; DROP ROLE regress_publication_user, regress_publication_user2; DROP ROLE regress_publication_user_dummy; -- 2.34.1 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-02-18 10:29 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-20 15:48 ` Re: Restrict publishing of partitioned table with a foreign table as partition vignesh C <[email protected]> 2025-03-24 09:33 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-03-24 15:47 ` Re: Restrict publishing of partitioned table with a foreign table as partition Álvaro Herrera <[email protected]> 2025-03-28 09:43 ` Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> @ 2025-03-28 11:04 ` Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Álvaro Herrera @ 2025-03-28 11:04 UTC (permalink / raw) To: Shlok Kyal <[email protected]>; +Cc: vignesh C <[email protected]>; Sergey Tatarintsev <[email protected]>; [email protected] On 2025-Mar-28, Shlok Kyal wrote: > On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera <[email protected]> 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/ ^ permalink raw reply [nested|flat] 9+ messages in thread
* [PATCH v2 1/2] Do not lock tables in get_tables_to_repack(). @ 2026-06-16 06:49 ChangAo Chen <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: ChangAo Chen @ 2026-06-16 06:49 UTC (permalink / raw) When doing a whole database repack, we build a list of repackable tables and take a lock on them to prevent concurrent drops. But concurrent drops can always happen after we build the list because we process each table in a separate transaction. The ConditionalLockRelationOid() also makes the default behavior like SKIP_LOCKED, which is unexpected. To remove the locks, we need to make repack_is_permitted_for_relation() handles concurrent drops correctly: it should not report an error when failing to search the syscache in pg_class_aclcheck(). Use pg_class_aclcheck_ext() instead to detect a concurrent drop. Also check the return value of get_rel_name(). While at it, replace relation_close() with table_close() to match the table_open(). --- src/backend/commands/repack.c | 67 ++++++++++------------------------- 1 file changed, 19 insertions(+), 48 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index faa07d1a118..2879c8af574 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -2169,22 +2169,9 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) index = (Form_pg_index) GETSTRUCT(tuple); - /* - * Try to obtain a light lock on the index's table, to ensure it - * doesn't go away while we collect the list. If we cannot, just - * disregard it. Be sure to release this if we ultimately decide - * not to process the table! - */ - if (!ConditionalLockRelationOid(index->indrelid, AccessShareLock)) - continue; - - /* Verify that the table still exists; skip if not */ classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(index->indrelid)); if (!HeapTupleIsValid(classtup)) - { - UnlockRelationOid(index->indrelid, AccessShareLock); continue; - } classForm = (Form_pg_class) GETSTRUCT(classtup); /* Skip temp relations belonging to other sessions */ @@ -2192,7 +2179,6 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) !isTempOrTempToastNamespace(classForm->relnamespace)) { ReleaseSysCache(classtup); - UnlockRelationOid(index->indrelid, AccessShareLock); continue; } @@ -2201,10 +2187,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) /* noisily skip rels which the user can't process */ if (!repack_is_permitted_for_relation(cmd, index->indrelid, GetUserId())) - { - UnlockRelationOid(index->indrelid, AccessShareLock); continue; - } /* Use a permanent memory context for the result list */ oldcxt = MemoryContextSwitchTo(permcxt); @@ -2228,45 +2211,20 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) class = (Form_pg_class) GETSTRUCT(tuple); - /* - * Try to obtain a light lock on the table, to ensure it doesn't - * go away while we collect the list. If we cannot, just - * disregard the table. Be sure to release this if we ultimately - * decide not to process the table! - */ - if (!ConditionalLockRelationOid(class->oid, AccessShareLock)) - continue; - - /* Verify that the table still exists */ - if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(class->oid))) - { - UnlockRelationOid(class->oid, AccessShareLock); - continue; - } - /* Can only process plain tables and matviews */ if (class->relkind != RELKIND_RELATION && class->relkind != RELKIND_MATVIEW) - { - UnlockRelationOid(class->oid, AccessShareLock); continue; - } /* Skip temp relations belonging to other sessions */ if (class->relpersistence == RELPERSISTENCE_TEMP && !isTempOrTempToastNamespace(class->relnamespace)) - { - UnlockRelationOid(class->oid, AccessShareLock); continue; - } /* noisily skip rels which the user can't process */ if (!repack_is_permitted_for_relation(cmd, class->oid, GetUserId())) - { - UnlockRelationOid(class->oid, AccessShareLock); continue; - } /* Use a permanent memory context for the result list */ oldcxt = MemoryContextSwitchTo(permcxt); @@ -2279,7 +2237,7 @@ get_tables_to_repack(RepackCommand cmd, bool usingindex, MemoryContext permcxt) } table_endscan(scan); - relation_close(catalog, AccessShareLock); + table_close(catalog, AccessShareLock); return rtcs; } @@ -2357,15 +2315,28 @@ get_tables_to_repack_partitioned(RepackCommand cmd, Oid relid, static bool repack_is_permitted_for_relation(RepackCommand cmd, Oid relid, Oid userid) { + bool is_missing = false; + Assert(cmd == REPACK_COMMAND_CLUSTER || cmd == REPACK_COMMAND_REPACK); - if (pg_class_aclcheck(relid, userid, ACL_MAINTAIN) == ACLCHECK_OK) + if (pg_class_aclcheck_ext(relid, userid, ACL_MAINTAIN, &is_missing) == ACLCHECK_OK) return true; - ereport(WARNING, - errmsg("permission denied to execute %s on \"%s\", skipping it", - RepackCommandAsString(cmd), - get_rel_name(relid))); + /* Report a warning if the relation still exists. */ + if (!is_missing) + { + char *relname; + + relname = get_rel_name(relid); + if (relname != NULL) + { + ereport(WARNING, + errmsg("permission denied to execute %s on \"%s\", skipping it", + RepackCommandAsString(cmd), relname)); + + pfree(relname); + } + } return false; } -- 2.47.3 --op6mnexl7cn72cto Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename=v2-0002-fixups.patch ^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-06-16 06:49 UTC | newest] Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-02-14 07:29 Re: Restrict publishing of partitioned table with a foreign table as partition Shlok Kyal <[email protected]> 2025-02-17 14:42 ` vignesh C <[email protected]> 2025-02-18 10:29 ` Shlok Kyal <[email protected]> 2025-02-20 15:48 ` vignesh C <[email protected]> 2025-03-24 09:33 ` Shlok Kyal <[email protected]> 2025-03-24 15:47 ` Álvaro Herrera <[email protected]> 2025-03-28 09:43 ` Shlok Kyal <[email protected]> 2025-03-28 11:04 ` Álvaro Herrera <[email protected]> 2026-06-16 06:49 [PATCH v2 1/2] Do not lock tables in get_tables_to_repack(). ChangAo Chen <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox