Thread

Re: Proposal: Conflict log history table for Logical Replication

Amit Kapila <amit.kapila16@gmail.com> — 2026-05-14T10:48:33Z

On Wed, May 13, 2026 at 11:43 AM Peter Smith <smithpb2250@gmail.com> 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.