On Tue, 1 Jul 2025 at 08:20, Ajin Cherian <itsa...@gmail.com> wrote:
>
> On Mon, Jun 9, 2025 at 3:58 PM Shlok Kyal <shlok.kyal....@gmail.com> wrote:
> >
> > On Wed, 4 Jun 2025 at 16:12, Ajin Cherian <itsa...@gmail.com> wrote:
> > >
> > > On Tue, May 20, 2025 at 2:33 AM Shlok Kyal <shlok.kyal....@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.
>
>
> Hi Shlok,
>
> Some more comments:
> 1.
> + Replication is not supported for foreign tables. When used as
> partitions
> + of partitioned tables, publishing of the partitioned table is only
> allowed
> + if the <literal>publish_via_partition_root</literal> is set to
> + <literal>false</literal>.
>
> In patch: "When used as partitions of partitioned tables, publishing
> of the partitioned table is only allowed if the
> <literal>publish_via_partition_root</literal> is set to
> <literal>false</literal>.
>
> Change to: When foreign tables are used as partitions of partitioned
> tables, publishing of the partitioned table is only allowed if the
> <literal>publish_via_partition_root</literal> is set to
> <literal>false</literal>.
>
Fixed
> 2.
> + <para>
> + When using a subscription parameter copy_data = true, corresponding
> + publications are checked if it has publish_via_partition_root = true and
> + has partitioned table with foreign table as partition. If this scenario
> is
> + detected an ERROR is logged to the user.
> + </para>
>
> In patch: "When using a subscription parameter copy_data = true, ..."
> Change to: "When using the subscription parameter copy_data = true, ..."
>
> In patch: "and has partitioned table with foreign table as partition."
> Change to: "and has a partitioned table with a foreign table as its partition"
>
Fixed
> 3.
> + * check_publications_foreign_parts
> + * Check if the publications, on which subscriber is subscribing, publishes
> any
> + * partitioned table that has an foreign table as its partition and has
> + * publish_via_partition_root set as true. The check is performed only if
> + * copy_data is set as true for the subscription.
>
> In patch: "publishes any partitioned table that has an foreign table
> as its partition"
> Change to: "publishes any partitioned table that has a foreign table
> as its partition"
>
Fixed
> 4.
> + * DML data changes are not published for data in foreign tables,
> + * and yet the tablesync worker is not smart enough to omit data from
> + * foreign tables when they are partitions of partitioned tables.
>
> change to:"Although DML changes to foreign tables are excluded from
> publication, the tablesync worker will still attempt to copy data from
> foreign table partitions during initial table synchronization."
>
Fixed
> 5.
> + * When publish_via_partition_root is false, each partition published for
> + * replication is listed individually in pg_subscription_rel, and we
> + * don't add partitions that are foreign tables, so this check is not
> + * needed.
>
> In patch: "so this check is not needed"
> Change to: "so this function is not called for such tables"
>
Fixed
> 6.
> + errdetail_plural("The subscription being created on a
> publication (%s) with publish_via_partition_root = true and contains
> partitioned tables with foreign table as partition ",
> + "The subscription being created on
> publications (%s) with publish_via_partition_root = true and contains
> partitioned tables with foreign table as partition ",
> + list_length(publist), pubnames->data),
>
> Change to: "The subscription is for a publication (%s) with
> publish_via_partition_root = true but one of the partitioned tables
> has a foreign table as a partition"
> "The subscription is for publications (%s) with
> publish_via_partition_root = true but one of the partitioned tables
> has a foreign table as a partition"
>
"one of the partitioned tables" this would be misleading as
publication can have multiple partitioned table which have foreign
table as its partition.
I have reworded it like:
"The subscription is for a publication (%s) with
publish_via_partition_root = true, but one or more partitioned tables
have foreign tables as partitions.",
"The subscription is for publications (%s) with
publish_via_partition_root = true but one or more partitioned tables
have foreign tables as partitions."
> 7.
> + /* capture all publications that include this relation directly */
> + puboids = list_concat(puboids, GetRelationPublications(rel->rd_id));
> + schemaid = RelationGetNamespace(rel);
> + puboids = list_concat(puboids, GetSchemaPublications(schemaid));
> +
> + /* and do the same for its ancestors, if any */
> + ancestors = get_partition_ancestors(rel->rd_id);
> + foreach_oid(ancestor, ancestors)
> + {
> + puboids = list_concat(puboids,
> GetRelationPublications(ancestor));
> + schemaid = get_rel_namespace(ancestor);
> + puboids = list_concat(puboids, GetSchemaPublications(schemaid));
> + }
> +
> + /* Now check the publish_via_partition_root bit for each of those */
> + list_sort(puboids, list_oid_cmp);
> + list_deduplicate_oid(puboids);
> + foreach_oid(puboid, puboids)
>
> Why do we need to do all this logic for non partition foreign tables?
> Just directly throw an error for those tables and do this extra logic
> only for partitioned tables.
>
We need to throw an error if foreign table is being attached to a
partitioned table which is already published (or its parent is already
published) with publish_via_partition_root is true.
So, this logic is required to check if the table to which it is being
attached is published with publish_via_partition_root or not.
>
> 8.
> + pg_fatal("Your installation contains publications, which has
> foreign table which are partitions of published table.\n"
> + "A list of potentially-affected publications is in
> the file:\n"
>
> Change to: "Your installation contains publications, where one of the
> partitioned tables has a foreign table as a partition.\n"
>
There can be mutiple partitioned table published. Reworded the error as:
"Your installation contains publications where one or more partitioned
tables have foreign tables as partitions.\n"
Thanks for reviewing the patch. I have addressed the comments and
attached the latest patch.
Thanks and Regards,
Shlok Kyal
v17-0001-Restrict-publishing-of-partitioned-table-with-fo.patch
Description: Binary data
