public inbox for [email protected]
help / color / mirror / Atom feedRe: Restrict publishing of partitioned table with a foreign table as partition
11+ messages / 5 participants
[nested] [flat]
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-02-14 07:29 Shlok Kyal <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-02-17 14:42 vignesh C <[email protected]>
parent: Shlok Kyal <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-02-18 10:29 Shlok Kyal <[email protected]>
parent: vignesh C <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-02-20 15:48 vignesh C <[email protected]>
parent: Shlok Kyal <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-03-24 09:33 Shlok Kyal <[email protected]>
parent: vignesh C <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-03-24 15:47 Álvaro Herrera <[email protected]>
parent: Shlok Kyal <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-03-28 09:43 Shlok Kyal <[email protected]>
parent: Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 11+ 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] 11+ messages in thread
* Re: Restrict publishing of partitioned table with a foreign table as partition
@ 2025-03-28 11:04 Álvaro Herrera <[email protected]>
parent: Shlok Kyal <[email protected]>
0 siblings, 0 replies; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ messages in thread
* Re: Do not lock tables in get_tables_to_repack
@ 2026-07-10 14:22 Álvaro Herrera <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Álvaro Herrera @ 2026-07-10 14:22 UTC (permalink / raw)
To: cca5507 <[email protected]>; +Cc: pgsql-hackers <[email protected]>
On 2026-Jul-08, cca5507 wrote:
> - classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(index->indrelid));
> + classtup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(index->indrelid));
>
> Do we really need to copy it? We hold a refcount on it so it won't be freed
> until we release it. Otherwise LGTM.
Yeah, I guess it doesn't matter. I have removed the copy and updated
some comments. I also realized that there are some places where we
weren't dealing correctly with the possibility that the relation goes
away, or is replaced with something different, so I added that too.
While looking at it I also realized that get_tables_to_repack_partitioned
is likewise not careful enough about it: we do IndexGetRelation(, false)
which fails hard if the pg_index tuple cannot be found, which is the
wrong thing to do.
At the same time, it's annoying that half of the code that clearly
belongs in that routine is actually in ExecRepack(). I moved that to
where it rightfully belongs. (The only somewhat annoying thing is that
we have to NULL-out the Relation pointer after returning; but that's not
*too* bad IMO.)
Any opinions on this?
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto,
no me acuerdo." (Augusto Pinochet a una corte de justicia)
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-07-10 14:22 UTC | newest]
Thread overview: 11+ 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]>
2026-06-16 06:49 [PATCH v2 1/2] Do not lock tables in get_tables_to_repack(). ChangAo Chen <[email protected]>
2026-07-10 14:22 Re: Do not lock tables in get_tables_to_repack Álvaro Herrera <[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