On Wed, May 13, 2026 at 11:43 AM Peter Smith <[email protected]> wrote:
>
> Some review comments for v33-0001.
>
> ======
> src/backend/catalog/aclchk.c
>
> pg_class_aclmask_ext:
>
> 1.
> if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
> ACL_USAGE)) &&
> - IsSystemClass(table_oid, classForm) &&
> - classForm->relkind != RELKIND_VIEW &&
> + IsConflictClass(classForm) &&
> !superuser_arg(roleid))
> - mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);
> + mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_USAGE);
> + else if ((mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE |
> ACL_TRUNCATE | ACL_USAGE)) &&
> + IsSystemClass(table_oid, classForm) &&
> + classForm->relkind != RELKIND_VIEW &&
> + !superuser_arg(roleid))
> + mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE | ACL_USAGE);
>
> The new patched code seems a bit repetitive.
>
> How about refactoring like below and putting the comments where they belong.
>
> if (!superuser_arg(roleid))
> {
> if (mask & (ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
> ACL_USAGE))
> {
> if (IsSystemClass(table_oid, classForm) &&
> classForm->relkind != RELKIND_VIEW)
> {
> /*
> * Deny anyone permission to update a system catalog unless
> * pg_authid.rolsuper is set.
> *
> * As of 7.4 we have some updatable system views; those shouldn't be
> * protected in this way. Assume the view rules can take care of
> * themselves. ACL_USAGE is if we ever have system sequences.
> */
> mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_DELETE | ACL_TRUNCATE |
> ACL_USAGE);
> }
> else if (IsConflictClass(classForm))
> {
> /*
> * For conflict log tables, we allow non-superusers to perform DELETE
> * and TRUNCATE for maintenance, while still restricting INSERT,
> * UPDATE, and USAGE.
> */
> mask &= ~(ACL_INSERT | ACL_UPDATE | ACL_USAGE);
> }
> }
> }
> else
> {
> /* Superusers bypass all permission-checking. */
>
> ReleaseSysCache(tuple);
> return mask;
> }
>
It is okay to reduce duplicity here but the check for IsConflictClass
should be first because IsSystemClass also contains the similar check
though for a different reason.
>
> 8.
> Despite some of these just being static, I am beginning to think that
> the "conflict" specific CLT code might be more appropriate to be put
> in conflict.c, along with the CLT schema etc.
>
> e.g. functions like:
> - create_conflict_log_table_tupdesc
> - create_conflict_log_table
> - GetLogDestination
>
+1.
>
> ======
> src/backend/replication/logical/conflict.c
>
> 13.
> +const char *const ConflictLogDestNames[] = {
> + [CONFLICT_LOG_DEST_LOG] = "log",
> + [CONFLICT_LOG_DEST_TABLE] = "table",
> + [CONFLICT_LOG_DEST_ALL] = "all"
> +};
> +
> +const ConflictLogColumnDef v[] = {
> + { .attname = "relid", .atttypid = OIDOID },
> + { .attname = "schemaname", .atttypid = TEXTOID },
> + { .attname = "relname", .atttypid = TEXTOID },
> + { .attname = "conflict_type", .atttypid = TEXTOID },
> + { .attname = "remote_xid", .atttypid = XIDOID },
> + { .attname = "remote_commit_lsn",.atttypid = LSNOID },
> + { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
> + { .attname = "remote_origin", .atttypid = TEXTOID },
> + { .attname = "replica_identity", .atttypid = JSONOID },
> + { .attname = "remote_tuple", .atttypid = JSONOID },
> + { .attname = "local_conflicts", .atttypid = JSONARRAYOID }
> +};
>
...
>
> 13c.
> TBH, I preferred code how it used to be -- where all the CLT constants
> and structs and enums and schemas were kept together. Now they are
> split across conflict.h and conflict.c making it harder to read as
> well as introducing need for static asserts that were not needed
> before.
>
That would lead to unnecessary inclusions at multiple places where it
is not required. See my 4th comment in email [1].
[1]:
https://www.postgresql.org/message-id/CAA4eK1LhOHa_TEznw%2BgFoq%2Bw0vMvvsDG2g9Xq8Mwa8xZMY73og%40mail.gmail.com
--
With Regards,
Amit Kapila.
