Re: Proposal: Conflict log history table for Logical Replication

Some review comments for v22-0001

======
src/backend/executor/execMain.c

1.
+ /*
+ * Conflict logging tables (CLT) are managed by the system to record
+ * replication conflicts. We allow DELETE to permit users to manually prune
+ * or truncate these logs, but manual data insertion or modification
+ * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of the
+ * system-generated logs.
+ */
+ if (IsConflictNamespace(RelationGetNamespace(resultRel)) &&
+ operation != CMD_DELETE)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("cannot execute %s on conflict logging table \"%s\"",
+ (operation == CMD_INSERT ? "INSERT" :
+ operation == CMD_UPDATE ? "UPDATE" :
+ operation == CMD_MERGE ? "MERGE" : "this operation"),
+ RelationGetRelationName(resultRel)),
+ errdetail("Conflict logging tables are system-managed and only
support cleanup via DELETE or TRUNCATE.")));

1a.
For consistency with every other place, "Conflict logging table"
should be changed to "Conflict log table". (See comment, errmsg,
errdetail).

~

1b.
Naming the failed operation seemed unnecessary. Also, the way it is
currently implemented, having "this operation" substitution looks like
bad practice for translations. Consider just a simpler message.

SUGGESTION
cannot modify or insert data for conflict log table \"%s\"

======
src/bin/psql/describe.c

2.
+ appendPQExpBuffer(&buf,
+ ", (CASE WHEN subconflictlogdest IN ('table', 'all') "
+ " THEN 'pg_conflict.pg_conflict_' || oid "
+ " ELSE '-' END) AS \"%s\"\n",
+ gettext_noop("Conflict log table"));

I'm not sure if you really needed to include the 'pg_conflict' schema
name here. It's just taking up space, and it's already well-defined in
the documentation that CLT only live in the 'pg_conflict' schema.

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