Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jun 30, 2026 at 7:26 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for v59-0001/0002.
>
> //////
> v59-0001
> //////
>
> ======
> src/backend/replication/logical/conflict.c
>
> 1.
> + *
> + * The tuple/key columns (replica_identity, remote_tuple, local_conflicts) are
> + * typed json rather than jsonb on purpose: they hold an exact audit snapshot
> + * of the applied tuples and replica identity, and json preserves the verbatim
> + * representation whereas jsonb would normalize it. Indexing them (jsonb's main
> + * advantage) wouldn't help anyway, as the conflict log is looked up by its
> + * scalar columns(relid, conflict_type, commit timestamp) while these JSON
> + * columns are per-conflict payload to inspect, not search keys.
> + */
>
> Use consistent case. This comment has a mixture of json and JSON.

Thanks, I will fix.

> ~~~
>
> 2.
> + { .attname = "remote_commit_lsn",.atttypid = LSNOID },
> + { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
> + { .attname = "remote_origin", .atttypid = TEXTOID },
> + { .attname = "replica_identity_full", .atttypid = BOOLOID },
> + { .attname = "replica_identity", .atttypid = JSONOID },
> + { .attname = "remote_tuple", .atttypid = JSONOID },
>
> The replica identity attributes are splitting the "remote" attributes.
> Isn't it better to keep all the remote attributes grouped together?

I prefer to keep all JSONOID together

> ~~~
>
> 3.
> +/*
> + * Convert the string representation of a conflict logging destination to its
> + * corresponding enum value.
> + */
> +ConflictLogDest
> +GetConflictLogDest(const char *dest)
> +{
> + /* NULL defaults to LOG. */
> + if (dest == NULL || pg_strcasecmp(dest, "log") == 0)
> + return CONFLICT_LOG_DEST_LOG;
>
> Instead of default/NULL always being equivalent of 'log', what are the
> thoughts about introducing a new GUC like
> `default_conflict_log_destination` to override that default?
>
> e.g
> * SUBCRIPTION option `conflict_log_destination` valid values can be
> "default" (default), "log", "table", "all"
> * option conflict_log_destination = "default" means use value from GUC
> `default_conflict_log_destination` which can be "log" (default) or
> "table" or "all".
>
> Existing code could then be configured to make use CLTs without having
> to touch the individual subscription DDLs.
> It might be useful when there are many subscriptions, or when a DBA
> wants to set a site-wide policy (e.g. always log to tables) without
> individual subscription management.

I do not have strong opinion on immediately providing GUC for this.

>
> //////
> v59-0002
> //////
>
> ======
> src/backend/catalog/aclchk.c
>
> 1.
> + /*
> + * Disallow creation in the conflict schema for everyone, including
> + * superusers, unless in binary-upgrade mode.
> + */
> + if (!IsBinaryUpgrade && (mask & ACL_CREATE) &&
> + IsConflictLogTableNamespace(nsp_oid))
> + return mask & ~ACL_CREATE;
> +
>
> Is this code OK? It only checks ACL_CREATE but how do you know the
> rest of the mask is sane?
>
> How about:
> return mask & ACL_ALL_RIGHTS_SCHEMA & ~ACL_CREATE;

I think this makes sense.

> ======
> src/backend/commands/tablecmds.c
>
> 2.
> + /*
> + * Conflict log tables are managed by the system for logical
> + * replication and should not be used as parent tables, as
> + * inheritance could interfere with the logging behavior.
> + */
> + if (IsConflictLogTableNamespace(relation->rd_rel->relnamespace))
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("permission denied: cannot inherit from conflict log table \"%s\"",
> + RelationGetRelationName(relation)),
> + errdetail("Conflict log tables are system-managed tables for logical
> replication conflicts.")));
>
> It is not really a lack of "privilege" preventing this -- it is just
> disallowed for everybody. Would ERRCODE_WRONG_OBJECT_TYPE be a more
> appropriate errcode?

I agree that we currently restrict most operations on CLTs, but the
issue isn't that the CLT is the wrong object type, they are still
relations. While we might open up certain operations in the future
based on business needs (such as allowing index creation), I still
believe this is a matter of privileges rather than object type.
Because these are internally managed system tables, no user has the
privilege to inherit from, alter, insert, update, or create indexes on
them. However, users *can* perform delete and truncate operations.
Therefore, the object type itself is correct; it is simply that users
currently lack the privileges to execute those specific operations.

--
Regards,
Dilip Kumar
Google