RE: bogus: logical replication rows/cols combinations

On Monday, May 16, 2022 2:10 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>
> On Fri, May 13, 2022 at 11:32 AM houzj(dot)fnst(at)fujitsu(dot)com
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Thursday, May 12, 2022 2:45 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > >
> > > On Wed, May 11, 2022 at 12:55 PM houzj(dot)fnst(at)fujitsu(dot)com
> > > <houzj(dot)fnst(at)fujitsu(dot)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