Re: Proposal: Conflict log history table for Logical Replication

On Wed, Jan 7, 2026 at 12:12 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Hi Dilip,
> Please find a few comments on v19-001:
>

Please find a few comments for v19-002's part I have reviewed so far:

1)
ReportApplyConflict:
+ if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
+ if ((dest & CONFLICT_LOG_DEST_LOG) != 0)

We can use IsSet everywhere

2)
GetConflictLogTableInfo
This function gets dest and opens table, shall we rename to :
GetConflictLogDestAndTable

3)
GetConflictLogTableInfo:
+ *log_dest = GetLogDestination(MySubscription->conflictlogdest);
+ conflictlogrelid = MySubscription->conflictlogrelid;
+
+ /* If destination is 'log' only, no table to open. */
+ if (*log_dest == CONFLICT_LOG_DEST_LOG)
+ return NULL;
+
+ Assert(OidIsValid(conflictlogrelid));

We don't need to fetch conflictlogrelid until after 'if (*log_dest ==
CONFLICT_LOG_DEST_LOG)' check. We shall move it after the 'if' check.

4)
GetConflictLogTableInfo:
+ /* Conflict log table is dropped or not accessible. */
+ if (conflictlogrel == NULL)
+ ereport(WARNING,
+ (errcode(ERRCODE_UNDEFINED_TABLE),
+ errmsg("conflict log table with OID %u does not exist",
+ conflictlogrelid)));

Shall we replace it with elog(ERROR)? IMO, it should never happen and
if it happens, we should raise it as an internal error as we do for
various other cases.


5)
ReportApplyConflict():

Currently the function structure is:

/* Insert to table if destination is 'table' or 'all' */
if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)

  /* Decide what detail to show in server logs. */
if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
else <table-only: put reduced info in log>


It will be good to make it:

/*
 * Insert to table if destination is 'table' or 'all' and
 * also log the error msg to serverlog
 */
if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
{...}
else <CONFLICT_LOG_DEST_LOG case>
{log complete detail}


6)
tuple_table_slot_to_indextup_json:
+ tuple = heap_form_tuple(tupdesc, values, isnull);

Do we need to do: heap_freetuple at the end?

thanks
Shveta