On Mon, Jun 15, 2026 at 10:35 AM Peter Smith <[email protected]> wrote:
>
> 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);
I do not really think we need a separate function or a new error code,
but I am open for what other thinks so for now I am not fixing it.
> ~
>
> 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
Yeah we can remove errdetails considering the other nearby permission
check code, provided no one else objects.
>
> 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*?
Based on our previous discussion, we are now allowing locking of the
conflict log tables. Therefore, this code block will be removed in
upcoming version and a note will be added to explain why this is
permitted.
--
Regards,
Dilip Kumar
Google
