MIME-Version: 1.0
References: 
 <CAA4eK1KQimd=0gLS1DHTK2HrZA3kPdaY3sDY9Jko8o-nXRQw9A@mail.gmail.com>
 <202205021144.zkbvyekn52v7@alvherre.pgsql>
 <CAA4eK1Le0Z6W_+oY+M4vV=fjwFMJRg3WguPr3LQAfEqATrSLbg@mail.gmail.com>
 <c19d48c7-d7ef-017b-41a9-7c5b000329b7@enterprisedb.com>
 <CAA4eK1JJ_cJZFanVFDC-1ipKC9aounno5JcWx1ceH035FZZ1MA@mail.gmail.com>
 <dea2d025-9084-fb59-c6ee-da2fc19985b1@enterprisedb.com>
 <CAA4eK1JC8v8ysNS9L1et0_yyZJOEE1wB+GrvOcar_f5dVadvBg@mail.gmail.com>
 <OS0PR01MB5716A594C58DE4FFD1F8100B94C89@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KYp1408g9P54iVX2uPwCvHR8R_8_H1oRZ9bv2iJvOOSw@mail.gmail.com>
 <OS0PR01MB571698DEFCE9ACC6DC40510794CA9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1+UCfKUaf41H6zHS2qR4CZoVtZe64yx_T-JVPSb1pdw_w@mail.gmail.com>
 <OS0PR01MB5716D2CF3F2E39A6246A76CE94CF9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB5716D2CF3F2E39A6246A76CE94CF9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Tue, 17 May 2022 12:22:45 +0530
Message-ID: 
 <CAA4eK1JhCpGdvBbyvs9RwJZeKe5TX1Whd6fSQ2VGUh6AUdWL2Q@mail.gmail.com>
Subject: Re: bogus: logical replication rows/cols combinations
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1JhCpGdvBbyvs9RwJZeKe5TX1Whd6fSQ2VGUh6AUdWL2Q%40mail.gmail.com>
Precedence: bulk

On Mon, May 16, 2022 at 6:04 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Attach the new version patch.
>

Few minor comments:
==================
1.
+      <para>
+       Names of table columns included in the publication. This contains all
+       the columns of table when user didn't specify column list for the
+       table.
+      </para></entry>

Can we slightly change it to: "Names of table columns included in the
publication. This contains all the columns of the table when the user
didn't specify the column list for the table."

2. Below comments needs to be removed from tablesync.c as we don't
combine column lists after this patch.

 * For initial synchronization, column lists can be ignored in following
* cases:
*
* 1) one of the subscribed publications for the table hasn't specified
* any column list
*
* 2) one of the subscribed publications has puballtables set to true
*
* 3) one of the subscribed publications is declared as ALL TABLES IN
* SCHEMA that includes this relation

3.
Note that we don't support the case where column list is different for
+ * the same table when combining publications. But we still need to check
+ * all the given publication-table mappings and report an error if any
+ * publications have different column list.

Can we change this comment to: "Note that we don't support the case
where the column list is different for the same table when combining
publications. But one can later change the publication so we still
need to check all the given publication-table mappings and report an
error if any publications have a different column list."?

4. Can we add a test for different column lists if it is not already there?

-- 
With Regards,
Amit Kapila.