On Wed, 4 Jun 2025 at 16:12, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, May 20, 2025 at 2:33 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > This approach seems better to me. I have created a patch with the
> > above approach.
> >
> > Thanks and Regards,
> > Shlok Kyal
>
> Some quick comments on the patch:
> 1. In doc/src/sgml/ref/create_subscription.sgml:
> + has partitioned table with foreign table as partition. If this scenario is
> + detected we ERROR is logged to the user.
> + </para>
> +
>
> Should be: "If this scenario is detected an ERROR is logged to the
> user." (remove "we").
>
> In src/backend/commands/subscriptioncmds.c:
> 2. The comment header:
> + * This function is in charge of detecting if publisher with
> + * publish_via_partition_root=true publishes a partitioned table that has a
> + * foreign table as a partition.
>
> Add "and throw an error if found" at the end of that sentence to
> correctly describe what the function does.
>
> 3.
> + appendStringInfoString(&cmd,
> + "SELECT DISTINCT P.pubname AS pubname "
> + "from pg_catalog.pg_publication p, LATERAL "
> + "pg_get_publication_tables(p.pubname) gpt, LATERAL "
> + "pg_partition_tree(gpt.relid) gt JOIN
> pg_catalog.pg_foreign_table ft ON "
> + "ft.ftrelid = gt.relid WHERE p.pubviaroot
> = true AND p.pubname IN (");
>
> use FROM rather than from to maintain SQL style consistency.
>
> 4.
> + errdetail_plural("The subscription being created on a
> publication (%s) with publish_via_root_partition = true and contains
> partitioned tables with foreign table as partition ",
> + "The subscription being created on
> publications (%s) with publish_via_root_partition = true and contains
> partitioned tables with foreign table as partition ",
> + list_length(publist), pubnames->data),
>
> I think you meant "publish_via_partition_root" here and not
> "publish_via_root_partition ".
>
I have addressed all the comments and attached the updated patch.
Thanks and Regards,
Shlok Kyal
