Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip/Vignesh.

Some review comments for patch v330003

======
Commit Message

1.
SELECT remote_xid, relname, remote_origin, local_conflicts[1] ->>
'xid' AS local_xid,
local_conflicts[1] ->> 'tuple' AS local_tuple
FROM myschema.conflict_log_history2;

~

Shouldn't this example be querying the pg_conflict schema (not
myschema), for a CLT name like pg_conflict_log_1234?

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

2.
+/* 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 }
+};

I think this all belongs directly beneath the ConflictLogSchema[]
where 'local_conflicts' was defined.

~~~

3.
+#define MAX_LOCAL_CONFLICT_INFO_ATTRS lengthof(LocalConflictSchema)

"MAX_" doesn't really seem appropriate as a prefix because this is not
some upper limit; it is just a number.

A better name is "NUM_LOCAL_CONFLICT_ATTRS".

~~

Ditto for the other "MAX_CONFLICT_ATTR_NUM" of patch 0001.

How about "NUM_CONFLICT_ATTRS".

~~~

RequestApplyConflict:

4.
+ if (dest == CONFLICT_LOG_DEST_TABLE || dest == CONFLICT_LOG_DEST_ALL)
+ log_dest_clt = true;
+ if (dest == CONFLICT_LOG_DEST_LOG || dest == CONFLICT_LOG_DEST_ALL)
+ log_dest_logfile = true;

This code could be improved by introducing some macros to hide all the
checking. There was also similar code in patch 0001 where such macros
would have been helpful.

SUGGESTION
log_dest_clt = CONFLICTS_LOGGED_TO_TABLE(dest);
log_dest_logfile = CONFLICTS_LOGGED_TO_FILE(dest);

~~~

5.
+ ereport(elevel,
+ errcode_apply_conflict(type),
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
+ get_namespace_name(RelationGetNamespace(localrel)),
+ RelationGetRelationName(localrel),
+ ConflictTypeNames[type]),
+ errdetail("Conflict details are logged to the conflict log table: %s",
+ RelationGetRelationName(conflictlogrel)));

I think there is some recently committed function for getting
fully-qualified relation names that this error can make use of.

~~~

6.
+ /* Standard reporting with full internal details. */
+ ereport(elevel,
+ errcode_apply_conflict(type),
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
+ get_namespace_name(RelationGetNamespace(localrel)),
+ RelationGetRelationName(localrel),
+ ConflictTypeNames[type]),
+ errdetail_internal("%s", err_detail.data));

Ditto. I think there is some recently committed function for getting
fully-qualified relation names that this error can make use of.

~~~

GetConflictLogDestAndTable:

7.
+ /* Quick exit if a conflict log table was not requested. */
+ if (*log_dest == CONFLICT_LOG_DEST_LOG)
+ return NULL;

It would be more intuitive to use that new macro here that I suggested
in a previous review comment.

SUGGESTION
if (!CONFLICTS_LOGGED_TO_TABLE(*log_dest))
return NULL;

~~~

InsertConflictLogTuple:

8.
+ int options = HEAP_INSERT_NO_LOGICAL;

This variable seems unnecessary. Easier to just pass
HEAP_INSERT_NO_LOGICAL as a function parameter.

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

start_apply:

9.
+ /* Open conflict log table and insert the tuple. */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);
+ Assert(dest != CONFLICT_LOG_DEST_LOG);
+ InsertConflictLogTuple(conflictlogrel);
+ table_close(conflictlogrel, RowExclusiveLock);

Another place where using the suggested new macro would be more intuitive.

SUGGESTION
Assert(CONFLICTS_LOGGED_TO_TABLE(dest));

======
src/test/subscription/t/035_conflicts.pl

10.
+# Verify the contents of the Conflict Log Table (CLT)
+# This section ensures that the clt contains the expected
+# type and specific key data.

/clt/CLT/

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