RE: bogus: logical replication rows/cols combinations

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:
> >
> > On Wednesday, May 11, 2022 11:33 AM Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > Fair enough, then we should go with a simpler approach to detect it
> > > in pgoutput.c (get_rel_sync_entry).
> >
> > OK, here is the patch that try to check column list in that way. The
> > patch also check the column list when CREATE SUBSCRIPTION and when
> starting initial copy.
> >
>
> 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.

> 2.
> + 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));
> + else
>
> Can we write comments to explain why we are using WARNING here instead of
> ERROR?
>
> 3.
> static void
> -pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry)
> +pgoutput_ensure_entry_cxt(PGOutputData *data, RelationSyncEntry *entry,
> + Relation relation)
>
> What is the need to change this interface as part of this patch?

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.

Best regards,
Hou zj