Re: bogus: logical replication rows/cols combinations

On Mon, May 16, 2022 at 6:50 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.