On Mon, Jun 1, 2026 at 7:10 AM Peter Smith <[email protected]> wrote: > > On Sun, May 31, 2026 at 9:54 PM Amit Kapila <[email protected]> wrote: > > > > On Sat, May 30, 2026 at 1:12 AM Dilip Kumar <[email protected]> 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
