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>
In-Reply-To: 
 <OS0PR01MB5716A594C58DE4FFD1F8100B94C89@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Thu, 12 May 2022 12:15:24 +0530
Message-ID: 
 <CAA4eK1KYp1408g9P54iVX2uPwCvHR8R_8_H1oRZ9bv2iJvOOSw@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/CAA4eK1KYp1408g9P54iVX2uPwCvHR8R_8_H1oRZ9bv2iJvOOSw%40mail.gmail.com>
Precedence: bulk

On Wed, May 11, 2022 at 12:55 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, May 11, 2022 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Fair enough, then we should go with a simpler approach to detect it in
> > pgoutput.c (get_rel_sync_entry).
>
> OK, here is the patch that try to check column list in that way. The patch also
> check the column list when CREATE SUBSCRIPTION and when starting initial copy.
>

Few comments:
===============
1.
initStringInfo(&cmd);
- appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename\n"
-    "  FROM pg_catalog.pg_publication_tables t\n"
+ appendStringInfoString(&cmd,
+    "SELECT DISTINCT t.schemaname,\n"
+    "                t.tablename,\n"
+    "                (CASE WHEN (array_length(pr.prattrs, 1) = t.relnatts)\n"
+    "                THEN NULL ELSE pr.prattrs END)\n"
+    "  FROM (SELECT P.pubname AS pubname,\n"
+    "               N.nspname AS schemaname,\n"
+    "               C.relname AS tablename,\n"
+    "               P.oid AS pubid,\n"
+    "               C.oid AS reloid,\n"
+    "               C.relnatts\n"
+    "          FROM pg_publication P,\n"
+    "          LATERAL pg_get_publication_tables(P.pubname) GPT,\n"
+    "          pg_class C JOIN pg_namespace N\n"
+    "                     ON (N.oid = C.relnamespace)\n"
+    "          WHERE C.oid = GPT.relid) t\n"
+    "  LEFT OUTER JOIN pg_publication_rel pr\n"
+    "       ON (t.pubid = pr.prpubid AND\n"
+    "        pr.prrelid = reloid)\n"

Can we modify pg_publication_tables to get the row filter and column
list and then use it directly instead of constructing this query?

2.
+ if (list_member(tablelist, rv))
+ ereport(WARNING,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use different column lists for table \"%s.%s\" in
different publications",
+    nspname, relname));
+ else

Can we write comments to explain why we are using WARNING here instead of ERROR?

3.
static void
-pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry)
+pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry,
+   Relation relation)

What is the need to change this interface as part of this patch?


-- 
With Regards,
Amit Kapila.