Re: Proposal: Conflict log history table for Logical Replication

On Mon, Jun 1, 2026 at 7:10 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sun, May 31, 2026 at 9:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sat, May 30, 2026 at 1:12 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> >
> > Few comments on 0001 and 0002
> > ===========================
> > 1.
> > + Oid         subconflictlogrelid; /* Relid of the conflict log table. */
> >  #ifdef CATALOG_VARLEN /* variable-length fields start here */
> > + /*
> > + * Strategy for logging replication conflicts:
> > + * 'log' - server log only,
> > + * 'table' - conflict log table only,
> > + * 'all' - both log and table.
> > + */
> > + text subconflictlogdest BKI_FORCE_NOT_NULL;
> >
> > 'log' sounds redundant in the above two field names. I feel naming
> > them as subconflictrelid and subconflictdest should be sufficient.
>
> I think removing the word 'log' would introduce ambiguity.
>
> IMO
> A "conflict" is a *thing* (e.g. constraint violation) that happened.
> And the "conflict log" is the *log* of the thing that happened.
>
> e.g. "subconflictlogrelid" is clearly the relid of the conflict log
> (CLT), but if it was just called "subconflictrelid" that sounds more
> like the relid of where the conflict occurred.

I agree with Peter that `subconflictlogrelid` makes more sense to
indicate this is the conflict log table's relid, not the conflicting
table's relid.  I do not have any strong opinion about
subconflictlogdest, IMHO subconflictlogdest, subconflictdest both
sounds fine.

> > 2. If you agree with the above, then let's make similar changes at
> > other places in the patch. We can change
> > alter_sub_conflictlogdestination to alter_sub_conflict_destination.
> > Also, similar to AlterSubscription_refresh and
> > AlterSubscription_refresh_seq, we can name this new function as
> > AlterSubscription_conflict_dest.
> >
> > 3. Now, let's consider whether we should change the option name to
> > conflict_data_destination instead of conflict_log_destination? The
> > reason I am asking to consider this change is that one of the option
> > values is 'log', so it sounded a bit odd to name the option as
> > conflict_log_destination. If we change this then we can consider
> > changing the name of Enum ConflictLogDest as well.

IMHO the 'log' in name conflict_log_destination doesn't really sounds
confusing with destination 'log'.  The conflict_log_destination
clearly indicates the destination for the conflict log whereas the log
in values "log/table/all" says the destination is server logs.

-- 
Regards,
Dilip Kumar
Google