Re: Proposal: Conflict log history table for Logical Replication

Hi Vignesh,

Some minor review comments for patches v37-0005/0006 combined.

======
src/backend/replication/logical/conflict.c

1.
+/* Schema for the elements within the 'local_conflicts' JSON array */
+static const ConflictLogColumnDef LocalConflictSchema[] =
+{
+ { .attname = "xid", .atttypid = XIDOID },
+ { .attname = "commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "origin", .atttypid = TEXTOID },
+ { .attname = "key", .atttypid = JSONOID },
+ { .attname = "tuple", .atttypid = JSONOID }
+};
+
+#define NUM_LOCAL_CONFLICT_ATTRS lengthof(LocalConflictSchema)
+

IMO this belongs *below* the ConflictLogSchema[], which is where
'local_conflicts' attribute was introduced, instead of above it.

~~~

2.
+
+
static int errcode_apply_conflict(ConflictType type);

~

There are some spurious blank lines here that should not be in the patch.

~~~

ProcessPendingConflictLogTuple:

3.
+ /* Open conflict log table and insert the tuple */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);
+ Assert(CONFLICTS_LOGGED_TO_TABLE(dest));

Maybe here it's better to say Assert(conflictlogrel);

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