MIME-Version: 1.0
References: 
 <CANhcyEVQ866NA5NbTi-aQhPj2zN_vbNb_ZW=E_P2foZYrW9VbA@mail.gmail.com>
 <202505101759.6qlloepaeddl@alvherre.pgsql>
 <CAA4eK1KavSbVdk9OeQDV5yc0FVy_rFTam5HPozqYSoM=a=8hXg@mail.gmail.com>
 <CANhcyEVUf5PEmahQvcc8GgCzxaAnJ8kTrir0Am_7O4SkZnpkQA@mail.gmail.com>
 <CAFPTHDYYq-Jx4RawpPNu78nq1TdPr7pZRWSdSm=eAejYr_wcag@mail.gmail.com>
 <CANhcyEVU9eqKtnL3vnNJ_fN7q5PwUwv2ceT8p7YaYKHedqLrpQ@mail.gmail.com>
In-Reply-To: 
 <CANhcyEVU9eqKtnL3vnNJ_fN7q5PwUwv2ceT8p7YaYKHedqLrpQ@mail.gmail.com>
From: Ajin Cherian <itsajin@gmail.com>
Date: Tue, 1 Jul 2025 12:50:41 +1000
Message-ID: 
 <CAFPTHDav-W4fF0ahCLTRQWxaywgDodv9_3cnBgDTOqv-9n1ywQ@mail.gmail.com>
Subject: Re: Restrict publishing of partitioned table with a foreign table as
 partition
To: Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
 =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@alvh.no-ip.org>,
	vignesh C <vignesh21@gmail.com>,
 Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>,
	pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAFPTHDav-W4fF0ahCLTRQWxaywgDodv9_3cnBgDTOqv-9n1ywQ%40mail.gmail.com>
Precedence: bulk

On Mon, Jun 9, 2025 at 3:58=E2=80=AFPM Shlok Kyal <shlok.kyal.oss@gmail.com=
> wrote:
>
> On Wed, 4 Jun 2025 at 16:12, Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Tue, May 20, 2025 at 2:33=E2=80=AFAM Shlok Kyal <shlok.kyal.oss@gmai=
l.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 sce=
nario 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=3Dtrue publishes a partitioned table tha=
t 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 =3D gt.relid WHERE p.pubviaroot
> > =3D 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 =3D true and contains
> > partitioned tables with foreign table as partition ",
> > +                                 "The subscription being created on
> > publications (%s) with publish_via_root_partition =3D 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 partit=
ions
+     of partitioned tables, publishing of the partitioned table is only al=
lowed
+     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>.

2.
+   <para>
+    When using a subscription parameter copy_data =3D true, corresponding
+    publications are checked if it has publish_via_partition_root =3D true=
 and
+    has partitioned table with foreign table as partition. If this scenari=
o is
+    detected an ERROR is logged to the user.
+   </para>

In patch: "When using a subscription parameter copy_data =3D true, ..."
Change to: "When using the subscription parameter copy_data =3D 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 partiti=
on"

3.
+ * check_publications_foreign_parts
+ * Check if the publications, on which subscriber is subscribing, publishe=
s 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"

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."

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"

6.
+                errdetail_plural("The subscription being created on a
publication (%s) with publish_via_partition_root =3D true and contains
partitioned tables with foreign table as partition ",
+                                 "The subscription being created on
publications (%s) with publish_via_partition_root =3D 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 =3D 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 =3D true but one of the partitioned tables
has a foreign table as a partition"

7.
+        /* capture all publications that include this relation directly */
+        puboids =3D list_concat(puboids, GetRelationPublications(rel->rd_i=
d));
+        schemaid =3D RelationGetNamespace(rel);
+        puboids =3D list_concat(puboids, GetSchemaPublications(schemaid));
+
+        /* and do the same for its ancestors, if any */
+        ancestors =3D get_partition_ancestors(rel->rd_id);
+        foreach_oid(ancestor, ancestors)
+        {
+            puboids =3D list_concat(puboids, GetRelationPublications(ances=
tor));
+            schemaid =3D get_rel_namespace(ancestor);
+            puboids =3D list_concat(puboids, GetSchemaPublications(schemai=
d));
+        }
+
+        /* 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.


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"

regards,
Ajin Cherian
Fujitsu Australia