On Thu, 13 Feb 2025 at 11:25, vignesh C <vignes...@gmail.com> wrote:
>
> On Tue, 11 Feb 2025 at 16:55, Shlok Kyal <shlok.kyal....@gmail.com> wrote:
> > I have handled the above cases and added tests for the same.
>
> There is a concurrency issue with the patch:
> +check_partrel_has_foreign_table(Form_pg_class relform)
> +{
> +       bool            has_foreign_tbl = false;
> +
> +       if (relform->relkind == RELKIND_PARTITIONED_TABLE)
> +       {
> +               List       *relids = NIL;
> +
> +               relids = find_all_inheritors(relform->oid, NoLock, NULL);
> +
> +               foreach_oid(relid, relids)
> +               {
> +                       Relation        rel = table_open(relid,
> AccessShareLock);
> +
> +                       if (RelationGetForm(rel)->relkind ==
> RELKIND_FOREIGN_TABLE)
> +                               has_foreign_tbl = true;
> +
> +                       table_close(rel, AccessShareLock);
> +
> +                       if (has_foreign_tbl)
> +                               break;
> +               }
> +       }
> +
> +       return has_foreign_tbl;
> +}
>
> In an ideal scenario, the creation of a foreign table should fail if
> there is an associated publication, as demonstrated below:
> CREATE TABLE t(id int) PARTITION BY RANGE(id);
> CREATE TABLE part1 PARTITION OF t FOR VALUES FROM (0) TO (5);
> CREATE TABLE part2 PARTITION OF t FOR VALUES FROM (5) TO (15)
> PARTITION BY RANGE(id);
> CREATE PUBLICATION pub1 FOR TABLE t with (publish_via_partition_root = true);
>
> postgres=# CREATE FOREIGN TABLE part22 PARTITION OF part2 FOR VALUES
> FROM (10) TO (15) SERVER fdw;
> ERROR:  cannot create table foreign partition "part22"
> DETAIL:  partition table "part2" is published with option
> publish_via_partition_root
>
> Consider a scenario where the publication is being created and after
> the check_partrel_has_foreign_table execution is done, concurrently
> creation of foreign table is executed, then the creation will be
> successful.
> postgres=# CREATE FOREIGN TABLE part22 PARTITION OF part2 FOR VALUES
> FROM (10) TO (15) SERVER fdw;
> CREATE FOREIGN TABLE
>
> I felt the problem here is that you have released the lock:
> +                       if (RelationGetForm(rel)->relkind ==
> RELKIND_FOREIGN_TABLE)
> +                               has_foreign_tbl = true;
> +
> +                       table_close(rel, AccessShareLock);
>
> We should retain the lock to fix this issue:
> +                       if (RelationGetForm(rel)->relkind ==
> RELKIND_FOREIGN_TABLE)
> +                               has_foreign_tbl = true;
> +
> +                       table_close(rel, NoLock);
>

Hi Vignesh,

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

Thanks and Regards,
Shlok Kyal

Attachment: v6-0001-Restrict-publishing-of-partitioned-table-with-for.patch
Description: Binary data

Reply via email to