Re: bogus: logical replication rows/cols combinations

On Fri, May 13, 2022 at 11:32 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, May 12, 2022 2:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, May 11, 2022 at 12:55 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> >
> > 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?
>
> Agreed. If we can get columnlist and rowfilter from pg_publication_tables, it
> will be more convenient. And I think users that want to fetch the columnlist
> and rowfilter of table can also benefit from it.
>

After the change for this, we will give an error on combining
publications where one of the publications specifies all columns in
the table and the other doesn't provide any columns. We should not
give an error as both mean all columns.

>
> Attach the new version patch which addressed these comments and update the
> document. 0001 patch is to extent the view and 0002 patch is to add restriction
> for column list.
>

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.

2.
 { oid => '6119', descr => 'get OIDs of tables in a publication',
   proname => 'pg_get_publication_tables', prorows => '1000', proretset => 't',
-  provolatile => 's', prorettype => 'oid', proargtypes => 'text',
-  proallargtypes => '{text,oid}', proargmodes => '{i,o}',
-  proargnames => '{pubname,relid}', prosrc => 'pg_get_publication_tables' },
+  provolatile => 's', prorettype => 'record', proargtypes => 'text',
+  proallargtypes => '{text,oid,int2vector,pg_node_tree}', proargmodes
=> '{i,o,o,o}',

I think we should change the "descr" to something like: 'get
information of tables in a publication'

3.
+
+ /*
+ * We only throw a warning here so that the subcription can still be
+ * created and let user aware that something is going to fail later and
+ * they can fix the publications afterwards.
+ */
+ 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));

Can we extend this comment to explain the case where after Alter
Publication, if the user dumps and restores back the subscription,
there is a possibility that "CREATE SUBSCRIPTION" won't work if we
give ERROR here instead of WARNING?

-- 
With Regards,
Amit Kapila.