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
