Re: bogus: logical replication rows/cols combinations

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?

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?

--
With Regards,
Amit Kapila.