Re: bogus: logical replication rows/cols combinations

On Mon, May 16, 2022 at 6:50 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-May-16, Amit Kapila wrote:
>
> > Few  comments:
> > =================
> > 1.
> > postgres=# select * from pg_publication_tables;
> >  pubname | schemaname | tablename | columnlist | rowfilter
> > ---------+------------+-----------+------------+-----------
> >  pub1    | public     | t1        |            |
> >  pub2    | public     | t1        | 1 2        | (c3 < 10)
> > (2 rows)
> >
> > I think it is better to display column names for columnlist in the
> > exposed view similar to attnames in the pg_stats_ext view. I think
> > that will make it easier for users to understand this information.
>
> +1
>

I have committed the first patch after fixing this part. It seems Tom
is not very happy doing this after beta-1 [1]. The reason we get this
information via this view (and underlying function) is that it
simplifies the queries on the subscriber-side as you can see in the
second patch. The query change is as below:
@@ -1761,17 +1762,18 @@ fetch_table_list(WalReceiverConn *wrconn, List
*publications)
  WalRcvExecResult *res;
  StringInfoData cmd;
  TupleTableSlot *slot;
- Oid tableRow[2] = {TEXTOID, TEXTOID};
+ Oid tableRow[3] = {TEXTOID, TEXTOID, NAMEARRAYOID};
  List    *tablelist = NIL;

  initStringInfo(&cmd);
- appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename\n"
+ appendStringInfoString(&cmd, "SELECT DISTINCT t.schemaname, t.tablename, \n"
+    "                      t.attnames\n"
     "  FROM pg_catalog.pg_publication_tables t\n"
     " WHERE t.pubname IN (");


Now, there is another way to change this query as well as done by
Hou-San in his first version [2] of the patch. The changed query with
that approach will be something like:
@@ -1761,17 +1762,34 @@ fetch_table_list(WalReceiverConn *wrconn, List
*publications)
  WalRcvExecResult *res;
  StringInfoData cmd;
  TupleTableSlot *slot;
- Oid tableRow[2] = {TEXTOID, TEXTOID};
+ Oid tableRow[3] = {TEXTOID, TEXTOID, INT2VECTOROID};
  List    *tablelist = NIL;

  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"

It appeared slightly complex and costly to me, so I have given the
suggestion to change it as we have now in the second patch as shown
above. Now, I can think of below ways to proceed here:

a. Revert the change in view (and underlying function) as done in
commit 0ff20288e1 and consider the alternate way (using a slightly
complex query) to fix. Then maybe for PG-16, we can simplify it by
changing the underlying function and view.
b. Proceed with the current approach of using a simplified query.

What do you think?

[1] - https://www.postgresql.org/message-id/91075.1652929852%40sss.pgh.pa.us
[2] -
https://www.postgresql.org/message-id/OS0PR01MB5716A594C58DE4FFD1F8100B94C89%40OS0PR01MB5716.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.