Re: bogus: logical replication rows/cols combinations

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.