Re: Proposal: Conflict log history table for Logical Replication

On Mon, Jun 1, 2026 at 9:03 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> > > 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.
>

Fair enough. But let's at least change function names as I suggest. I
would like to suggest changing GetLogDestination to
GetConflictLogDest(). drop_sub_dependencies() is too generi, the
function only drops the conflict log tab, so, calling it
drop_sub_dependencies implies it handles all subscription
dependencies. drop_sub_conflict_log_table() is more accurate and won't
confuse future readers who see it called from DropSubscription.
CONFLICTS_LOGGED_TO_FILE(): "File" is not what the server log is
called anywhere in PostgreSQL, it should be CONFLICTS_LOGGED_TO_LOG().
Additionally, IsConflictLogTableClass() vs IsConflictNamespace()
sounds inconsistent.

-- 
With Regards,
Amit Kapila.