RE: bogus: logical replication rows/cols combinations

On Monday, May 16, 2022 2:10 PM Amit Kapila <amit.kapila16@gmail.com>
> 
> 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.

Thanks for the comments. Fixed.

> >
> > 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.

Agreed and changed.

 
> 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'

Changed.

> 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?

After rethinking about this, It seems ok to report an ERROR here as the pg_dump
of subscription always set (connect = false). So, we won't hit the check when
restore the dump which means the restore can be successful even if user change
the publication afterwards. Based on this, I have changed the warning to error.

Attach the new version patch.

Best regards,
Hou zj