On Monday, May 16, 2022 2:10 PM Amit Kapila <[email protected]> > > On Fri, May 13, 2022 at 11:32 AM [email protected] > <[email protected]> wrote: > > > > On Thursday, May 12, 2022 2:45 PM Amit Kapila <[email protected]> > wrote: > > > > > > On Wed, May 11, 2022 at 12:55 PM [email protected] > > > <[email protected]> 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
v2-0002-Prohibit-combining-publications-with-different-colum.patch
Description: v2-0002-Prohibit-combining-publications-with-different-colum.patch
v2-0001-Extend-pg_publication_tables-to-display-column-list-.patch
Description: v2-0001-Extend-pg_publication_tables-to-display-column-list-.patch
