Re: Proposal: Conflict log history table for Logical Replication

Some review comments for patch v21-0001.

======
src/backend/catalog/pg_publication.c

GetAllPublicationRelations:

1.
+ /* Subscription conflict log tables are not published */
  if (is_publishable_class(relid, relForm) &&
  !(relForm->relispartition && pubviaroot))
  result = lappend_oid(result, relid);
You replied that this (redundant) comment had been removed, but it is
still in the patch.

======
src/backend/commands/subscriptioncmds.c

2.
+ ConflictLogDest logdest;
  XLogRecPtr lsn;
 } SubOpts;

Should you call this member 'conflictlogdest', in case the future
supports other kinds of subscription logging?

======
src/include/replication/conflict.h

3.
+static const ConflictLogColumnDef ConflictLogSchema[] =
+{
+ { .attname = "relid",            .atttypid = OIDOID },
+ { .attname = "schemaname",       .atttypid = TEXTOID },
+ { .attname = "relname",          .atttypid = TEXTOID },
+ { .attname = "conflict_type",    .atttypid = TEXTOID },
+ { .attname = "remote_xid",       .atttypid = XIDOID },
+ { .attname = "remote_commit_lsn",.atttypid = LSNOID },
+ { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "remote_origin",    .atttypid = TEXTOID },
+ { .attname = "replica_identity", .atttypid = JSONOID },
+ { .attname = "remote_tuple",     .atttypid = JSONOID },
+ { .attname = "local_conflicts",  .atttypid = JSONARRAYOID }
+};

Is "local_conflicts" meant to be a JSON array or a JSONB array? I
think there are lots of comments in the patch 0002 describing it as
"JSONB", but I don't know if those comments are accurate or not. If
those comments are correct, then shouldn't it be saying JSONBARRAYOID
here?

TBH, I think the JSONOID/JSONARRAYOID is correct, but something is
fishy and clashing with patch 0002.

~~~

4.
+ /* Convenience flag for all supported destinations */
+ CONFLICT_LOG_DEST_ALL   = (CONFLICT_LOG_DEST_LOG | CONFLICT_LOG_DEST_TABLE)
+} ConflictLogDest;

Is this better described as a "bitmask", instead of a "flag"?

======
src/test/regress/sql/subscription.sql

5.
+DROP SUBSCRIPTION regress_conflict_protection_test;
+
+
 RESET SESSION AUTHORIZATION;

Double blank lines.

======
Kind Regards,
Peter Smith.
Fujitsu Australia