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

From: vignesh C <[email-masked]>
Date: Wed, 5 Feb 2025 12:12:40 +0530

On Tue, 4 Feb 2025 at 21:21, Álvaro Herrera <[email-masked]> wrote:
>
> On 2025-Feb-04, vignesh C wrote:
>
> > We should throw an error for partitioned tables that contain foreign
> > partitions, as this would include the data from these foreign tables
> > during the initial sync, while incremental changes would not be
> > replicated.
>
> Hmm, I would support the idea of allowing partitioned tables containing
> foreign partitions into publications, where only the tuples in
> non-foreign partitions would be transmitted -- both during initial sync
> and during replicated DML.  This way, any partitioned tables with mixed
> local and foreign partitions would work okay for replication.  If the
> subscriber wants the data in the foreign partitions, they can create the
> foreign table on the subscription side and all is well.  From a users
> POV this is probably the most useful.
>
> Another aspect is the case where you create the publication first and
> the foreign table later.  I didn't spot in the proposed patch any code
> that would check whether a publication including this schema exists,
> which we would have to do if we wanted to be watertight about rejecting
> publications with foreign tables.  But I think that would be a bad
> direction to go it.
>
> I'd rather have the table-listing code for publications skip any foreign
> tables.  For instance, GetPubPartitionOptionRelations() and
> PublicationAddTables() should skip tables with relkind RELKIND_FOREIGN.
> The sync code should also silently ignore all foreign tables.

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

Regards,
Vignesh

---

From: Álvaro Herrera <[email-masked]>
Date: Wed, 5 Feb 2025 09:44:40 +0100

> foreign table data. In this case, wouldn’t it be better to throw an

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

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

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

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

---

From: Shlok Kyal <[email-masked]>
Date: Tue, 11 Feb 2025 16:55:15 +0530

> that changes are published directly by partitions, which is sensible
I have created a patch with the above approach.
We can create a publication on a partition table with foreign
partition when publish_via_partition_root=false. In this case foreign
partitions will not be published. And if
publish_via_partition_root=true we will throw an error.
Please find the v5 patch.

> TABLES that would be affected.

I have handled the above cases and added tests for the same.

> skip the tuples in foreign partitions, it's easy to remove the

Thanks and Regards,
Shlok Kyal

---

From: vignesh C <[email-masked]>
Date: Thu, 13 Feb 2025 11:24:51 +0530

> I have handled the above cases and added tests for the same.

There is a concurrency issue with the patch:
+check_partrel_has_foreign_table(Form_pg_class relform)
+{
+       bool            has_foreign_tbl = false;
+
+       if (relform->relkind == RELKIND_PARTITIONED_TABLE)
+       {
+               List       *relids = NIL;
+
+               relids = find_all_inheritors(relform->oid, NoLock, NULL);
+
+               foreach_oid(relid, relids)
+               {
+                       Relation        rel = table_open(relid,
AccessShareLock);
+
+                       if (RelationGetForm(rel)->relkind ==
RELKIND_FOREIGN_TABLE)
+                               has_foreign_tbl = true;
+
+                       table_close(rel, AccessShareLock);
+
+                       if (has_foreign_tbl)
+                               break;
+               }
+       }
+
+       return has_foreign_tbl;
+}

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

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

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

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

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

Regards,
Vignesh

---

From: Shlok Kyal <[email-masked]>
Date: Thu, 13 Feb 2025 15:50:38 +0530

> +                       table_close(rel, NoLock);

Hi Vignesh,

I have fixed the issue. Attached the updated v6 patch.

Thanks and Regards,
Shlok Kyal