Re: Proposal: Conflict log history table for Logical Replication

Some review comments for v50-0002.

======
Commit message

1.
Missing commit message.

======
GENERAL

2.
+ if (IsConflictLogTableClass(rel->rd_rel))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a conflict log table",
+ RelationGetRelationName(rel)),
+ errdetail("Conflict log tables are system-managed tables for logical
replication conflicts.")));

2a.
There are many duplicate "permission denied" messages in this patch,
like above. Is it possible to do something like introduce an
OBJECT_SUBSCRIPTION_CLT so you can delegate all these to common
aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_SUBSCRIPTION_CLT, clt_name)
error processing?

Or, if that's a bit too hacky, put a common error function in catalog.c.

e.g.
if (IsConflictLogTableClass(rel->rd_rel))
  ConflictLogTablePermissionDenied(rel);

~

2b.
I'm not sure that the errdetail is really needed, since none of the
nearby messages have extra errdetail. Instead, you could just modify
the errmsg like:
"permission denied: \"%s\" is a system-managed conflict log table".

======
src/backend/commands/lockcmds.c

3.
+ if (IsConflictLogTableNamespace(get_rel_namespace(relid)) &&
+ lockmode > AccessShareLock)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied: \"%s\" is a conflict log table",
+ rv->relname),
+ errdetail("Conflict log tables are system-managed and cannot be
locked explicitly, except in ACCESS SHARE mode.")));
+

Is ERRCODE_INSUFFICIENT_PRIVILEGE/"permission denied" the correct
errcode/errmsg to use here. e.g.,  is lack of privilege really the
problem given it was not supposed to be locked by *anybody*?

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