Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

On Mon, Dec 22, 2025 at 4:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> Done in V15

Thanks for the patches. A few comments on v15-002 for the part I have
reviewed so far:

1)
Defined twice:

+#define MAX_LOCAL_CONFLICT_INFO_ATTRS 5

+#define MAX_LOCAL_CONFLICT_INFO_ATTRS \
+ (sizeof(LocalConflictSchema) / sizeof(LocalConflictSchema[0]))


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

We can get conflictlogrelid after the if-check for DEST_LOG.

3)
In ReportApplyConflict(), we form err_detail by calling
errdetail_apply_conflict(). But when dest is TABLE, we don't use
err_detail. Shall we skip creating it for dest=TABLE case?

4)
ReportApplyConflict():
+ /*
+ * Get both the conflict log destination and the opened conflict log
+ * relation for insertion.
+ */
+ conflictlogrel = GetConflictLogTableInfo(&dest);
+

We can move it after errdetail_apply_conflict(), closer to where we
actually use it.

5)
start_apply:
+ /* Open conflict log table and insert the tuple. */
+ conflictlogrel = GetConflictLogTableInfo(&dest);
+ if (ValidateConflictLogTable(conflictlogrel))
+ InsertConflictLogTuple(conflictlogrel);

We can have Assert here too before we call Validate:
Assert(dest == CONFLICT_LOG_DEST_TABLE || dest == CONFLICT_LOG_DEST_ALL);

6)
start_apply:
+ if (ValidateConflictLogTable(conflictlogrel))
+ InsertConflictLogTuple(conflictlogrel);
+ MyLogicalRepWorker->conflict_log_tuple = NULL;

InsertConflictLogTuple() already sets conflict_log_tuple to NULL.
Above is not needed.

thanks
Shveta