Re: Proposal: Conflict log history table for Logical Replication

Some review comments for v20-0002 (code)

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

ReportApplyConflict:

1.
+ /* Insert to table if requested. */
+ if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
+ {
...
+ }

+ /* Decide what detail to show in server logs. */
+ if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
+ {
...
+ /* 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));
+ }
+ else
+ {
+ /*
+ * 'table' only: Report the error msg but omit raw tuple data from
+ * server logs since it's already captured in the internal table.
+ */
+ 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 logged to internal table with OID %u.",
+ MySubscription->conflictlogrelid));
+ }

OK, I understand better now what you are trying to do with the logging
-- e.g. I see the brief log is referring to the CLT. But, it seems a
bit strange still that the 'else' of the LOG is asserting there *must*
be a CLT. Perhaps that is a valid assumption today, but if another
kind of destination is supported in the future, then I doubt this
logic is correct.

How about using some variables, like this:

SUGGESTION

bool log_dest_clt = (dest & CONFLICT_LOG_DEST_TABLE) != 0);
bool log_dest_logfile = (dest & CONFLICT_LOG_DEST_LOG) != 0);

if (log_dest_clt)
{
/* Logging to the CLT */
...

if (!log_dest_logfile)
{
/* Not logging conflict details to the server log; instead, write
a brief message referencing this CLT. */
ereport(elevel, ...
errdetail("Conflict details logged to internal table with OID %u.", ...);
}
}

if (log_dest_logfile)
{
/* Logging to the Server Log */
...
}

~~~

build_local_conflicts_json_array:

2.
+ json_datum_array = (Datum *) palloc_array(Datum, num_conflicts);
+ json_null_array = (bool *) palloc0_array(bool, num_conflicts);

The casts are redundant. The macros are taking care of that.

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

3.
-extern Relation GetConflictLogTableInfo(ConflictLogDest *log_dest);
+extern Relation GetConflictLogDestAndTable(ConflictLogDest *log_dest);
+extern void InsertConflictLogTuple(Relation conflictlogrel);
+extern bool ValidateConflictLogTable(Relation rel);

3a.
AFAICT GetConflictLogTableInfo() should not have existed anyway.

~

3b.
AFAICT ValidateConflictLogTable() is not used anymore. This seems
leftover from some old patch version.

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