MIME-Version: 1.0
References: <202504062027.tqmabk2h353o@alvherre.pgsql>
 <c64352fa-9a30-4e0a-853a-a6b5b6d07f4e@postgrespro.ru>
In-Reply-To: <c64352fa-9a30-4e0a-853a-a6b5b6d07f4e@postgrespro.ru>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Mon, 28 Apr 2025 18:21:46 +0530
Message-ID: 
 <CANhcyEU9Bq9G7ib1ZH7Zp=LN_M-d7=PrDG9NcDhe5JRi6dwedg@mail.gmail.com>
Subject: Re: Restrict publishing of partitioned table with a foreign table as
 partition
To: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@alvh.no-ip.org>,
	vignesh C <vignesh21@gmail.com>, 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/CANhcyEU9Bq9G7ib1ZH7Zp%3DLN_M-d7%3DPrDG9NcDhe5JRi6dwedg%40mail.gmail.com>
Precedence: bulk

On Mon, 7 Apr 2025 at 09:43, Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> 07.04.2025 03:27, =C3=81lvaro Herrera =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
>
> On 2025-Apr-01, Shlok Kyal wrote:
>
> I have modified the comment in create_publication.sgml and also added
> comment in the restrictions section of logical-replication.sgml.
> I have also added a more detailed explanation in comment of
> 'check_foreign_tables'
>
> I have attached the updated v11 patch.
>
> Sadly I don't have time to describe the changes proposed here right now,
> but I'll do that early tomorrow.  (Some minor changes are still needed,
> particularly the comments to publication_check_foreign_parts which are
> mostly unchanged from what your patch has.  I'll do another review round
> tomorrow.)
>
> Hello!
>
> I looked at the latest patch again and found one more place for list_free=
(). Also look at additional test case:
>
> diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/p=
g_publication.c index 51e463c112b..7fcc191feb9 100644 --- a/src/backend/cat=
alog/pg_publication.c +++ b/src/backend/catalog/pg_publication.c @@ -442,6 =
+442,7 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt p=
ub_partopt,                           result =3D lappend_oid(result, partOi=
d);                 } +               list_free(all_parts);         }      =
   else                 result =3D lappend_oid(result, relid); diff --git a=
/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sq=
l index 49c9d98b668..e56aebc397a 100644 --- a/src/test/regress/sql/publicat=
ion.sql +++ b/src/test/regress/sql/publication.sql @@ -1296,6 +1296,14 @@ S=
ELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('s=
ch3'  -- foreign partition  ALTER PUBLICATION pub1 SET (publish_via_partiti=
on_root);   +CREATE SCHEMA sch5; +CREATE SCHEMA sch6; +CREATE TABLE sch6.tm=
ain(id int) PARTITION BY RANGE(id); +CREATE TABLE sch5.part1 PARTITION OF s=
ch6.tmain FOR VALUES FROM (0) TO (10) PARTITION BY RANGE(id); +CREATE FOREI=
GN TABLE sch6.part2 PARTITION OF sch5.part1 FOR VALUES FROM (0) TO (5) SERV=
ER fdw_server; +CREATE PUBLICATION pub4 FOR TABLES IN SCHEMA sch5 WITH (pub=
lish_via_partition_root); +SELECT pubname, tablename FROM pg_publication_ta=
bles WHERE pubname =3D 'pub4' ORDER BY pubname, tablename; +  DROP PUBLICAT=
ION pub1;  DROP PUBLICATION pub2;  DROP PUBLICATION pub3;
>
> I think this is a wrong assumption:
>
> ScanKeyInit(&key[keycount++], Anum_pg_class_relispartition, BTEqualStrate=
gyNumber, F_BOOLEQ, BoolGetDatum(false));
>
> In this case sch5.part1 is partitioned table, but it also partition of ta=
ble in different schema
>

Hi Sergey,

Thanks for reviewing the patch. I have addressed the comment in the
latest patch shared in [1].

[1]: https://www.postgresql.org/message-id/CANhcyEW0QMiJXMqpPFRHni-q0Rm4R0h=
pZ0LdaqA%3DF3wvDUU6sQ%40mail.gmail.com

Thanks and Regards,
Shlok Kyal