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

On Thu, 13 Feb 2025 at 11:25, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Tue, 11 Feb 2025 at 16:55, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)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