Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jun 30, 2026 at 9:22 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Jun 30, 2026 at 7:26 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > ~~~
> >
> > 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
>

The other argument is that currently the scalar columns are before the
payload data which is at the end, so the current organisation of
columns makes more sense to me.

> > ~~~
> >
> > 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.
>

I feel as a first version, we don't need more configurable options. We
can extend it in the future seeing the usage in the real-world.

>
> > ======
> > 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.
>

If we look from this POV where we want to support some features in
future, ERRCODE_FEATURE_NOT_SUPPORTED sounds like a better option.
OTOH, in execMain.c, in 0001, WRONG_OBJECT_TYPE is any way a better
fit because we never want to allow INSERT/UPDATE/MERGE. Having, single
error_code makes it easy to understand/explain/extend this feature,
otherwise, the future users will always be confused as to which
error_code is better fit. I think insuffcient_privilige doesn't suit
even if we want to support such features in future because those won't
require a new grant kind of privilege. Even though WRONG_OBJECT_TYPE
is slightly over stretched for some cases but I think consistency
helps, so, I would prefer WRONG_OBJECT_TYPE in the case Peter pointed
out and other DDL operations where we have a similar check.

--
With Regards,
Amit Kapila.