Content-Type: multipart/alternative;
 boundary="------------DT9MqFBxCqAXH4YOSF3tp5Qb"
Message-ID: <c64352fa-9a30-4e0a-853a-a6b5b6d07f4e@postgrespro.ru>
Date: Mon, 7 Apr 2025 11:13:04 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Restrict publishing of partitioned table with a foreign table as
 partition
To: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@alvh.no-ip.org>,
 Shlok Kyal <shlok.kyal.oss@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, pgsql-hackers@lists.postgresql.org
References: <202504062027.tqmabk2h353o@alvherre.pgsql>
Content-Language: en-US
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
In-Reply-To: <202504062027.tqmabk2h353o@alvherre.pgsql>
Archived-At: <https://www.postgresql.org/message-id/c64352fa-9a30-4e0a-853a-a6b5b6d07f4e%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------DT9MqFBxCqAXH4YOSF3tp5Qb
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

07.04.2025 03:27, Álvaro Herrera пишет:
> 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/pg_publication.c index 51e463c112b..7fcc191feb9 
100644 --- a/src/backend/catalog/pg_publication.c +++ 
b/src/backend/catalog/pg_publication.c @@ -442,6 +442,7 @@ 
GetPubPartitionOptionRelations(List *result, PublicationPartOpt 
pub_partopt,                         result = lappend_oid(result, 
partOid);                 } +               list_free(all_parts); 
         }         else                 result = lappend_oid(result, 
relid); diff --git a/src/test/regress/sql/publication.sql 
b/src/test/regress/sql/publication.sql index 49c9d98b668..e56aebc397a 
100644 --- a/src/test/regress/sql/publication.sql +++ 
b/src/test/regress/sql/publication.sql @@ -1296,6 +1296,14 @@ SELECT 
pubname, tablename FROM pg_publication_tables WHERE schemaname in 
('sch3'  -- foreign partition  ALTER PUBLICATION pub1 SET 
(publish_via_partition_root); +CREATE SCHEMA sch5; +CREATE SCHEMA sch6; 
+CREATE TABLE sch6.tmain(id int) PARTITION BY RANGE(id); +CREATE TABLE 
sch5.part1 PARTITION OF sch6.tmain FOR VALUES FROM (0) TO (10) PARTITION 
BY RANGE(id); +CREATE FOREIGN TABLE sch6.part2 PARTITION OF sch5.part1 
FOR VALUES FROM (0) TO (5) SERVER fdw_server; +CREATE PUBLICATION pub4 
FOR TABLES IN SCHEMA sch5 WITH (publish_via_partition_root); +SELECT 
pubname, tablename FROM pg_publication_tables WHERE pubname = 'pub4' 
ORDER BY pubname, tablename; +  DROP PUBLICATION pub1;  DROP PUBLICATION 
pub2;  DROP PUBLICATION pub3;

I think this is a wrong assumption:

ScanKeyInit(&key[keycount++], Anum_pg_class_relispartition, 
BTEqualStrategyNumber, F_BOOLEQ, BoolGetDatum(false));

In this case sch5.part1 is partitioned table, but it also partition of 
table in different schema

-- 
With best regards,
Sergey Tatarintsev,
PostgresPro

--------------DT9MqFBxCqAXH4YOSF3tp5Qb
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">07.04.2025 03:27, Álvaro Herrera пишет:<br>
    </div>
    <blockquote type="cite"
      cite="mid:202504062027.tqmabk2h353o@alvherre.pgsql">
      <pre wrap="" class="moz-quote-pre">On 2025-Apr-01, Shlok Kyal wrote:

</pre>
      <blockquote type="cite">
        <pre wrap="" class="moz-quote-pre">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.
</pre>
      </blockquote>
      <pre wrap="" class="moz-quote-pre">
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.)

</pre>
    </blockquote>
    <p>Hello!</p>
    <p>I looked at the latest patch again and found one more place for list_free(). Also look at additional test case:
</p>
    <p>diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index 51e463c112b..7fcc191feb9 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -442,6 +442,7 @@ GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,
 
                        result = lappend_oid(result, partOid);
                }
+               list_free(all_parts);
        }
        else
                result = lappend_oid(result, relid);
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 49c9d98b668..e56aebc397a 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -1296,6 +1296,14 @@ SELECT pubname, tablename FROM pg_publication_tables WHERE schemaname in ('sch3'
 -- foreign partition
 ALTER PUBLICATION pub1 SET (publish_via_partition_root);
 
+CREATE SCHEMA sch5;
+CREATE SCHEMA sch6;
+CREATE TABLE sch6.tmain(id int) PARTITION BY RANGE(id);
+CREATE TABLE sch5.part1 PARTITION OF sch6.tmain FOR VALUES FROM (0) TO (10) PARTITION BY RANGE(id);
+CREATE FOREIGN TABLE sch6.part2 PARTITION OF sch5.part1 FOR VALUES FROM (0) TO (5) SERVER fdw_server;
+CREATE PUBLICATION pub4 FOR TABLES IN SCHEMA sch5 WITH (publish_via_partition_root);
+SELECT pubname, tablename FROM pg_publication_tables WHERE pubname = 'pub4' ORDER BY pubname, tablename;
+
 DROP PUBLICATION pub1;
 DROP PUBLICATION pub2;
 DROP PUBLICATION pub3;
</p>
    <p>I think this is a wrong assumption:
</p>
    <p>	ScanKeyInit(&amp;key[keycount++],
				Anum_pg_class_relispartition,
				BTEqualStrategyNumber, F_BOOLEQ,
				BoolGetDatum(false));</p>
    <p>In this case sch5.part1 is partitioned table, but it also partition of table in different schema
<style type="text/css">p, li { white-space: pre-wrap; }</style></p>
    <pre class="moz-signature" cols="72">-- 
With best regards,
Sergey Tatarintsev, 
PostgresPro</pre>
  </body>
</html>

--------------DT9MqFBxCqAXH4YOSF3tp5Qb--