On Mon, Jun 1, 2026 at 9:03 AM Dilip Kumar <[email protected]> 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.
