Horton

Re: Proposal: Conflict log history table for Logical Replication

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, shveta malik <shveta.malik@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, shveta malik <shvetamalik@gmail.com>
Date: 2026-05-21T05:51:02Z
Lists: pgsql-hackers 
Hi Vignesh,

Some minor review comments for patches v37-0005/0006 combined.

======
src/backend/replication/logical/conflict.c

1.
+/* Schema for the elements within the 'local_conflicts' JSON array */
+static const ConflictLogColumnDef LocalConflictSchema[] =
+{
+ { .attname = "xid",       .atttypid = XIDOID },
+ { .attname = "commit_ts", .atttypid = TIMESTAMPTZOID },
+ { .attname = "origin",    .atttypid = TEXTOID },
+ { .attname = "key",       .atttypid = JSONOID },
+ { .attname = "tuple",     .atttypid = JSONOID }
+};
+
+#define NUM_LOCAL_CONFLICT_ATTRS lengthof(LocalConflictSchema)
+

IMO this belongs *below* the ConflictLogSchema[], which is where
'local_conflicts' attribute was introduced, instead of above it.

~~~

2.
+
+
 static int errcode_apply_conflict(ConflictType type);

~

There are some spurious blank lines here that should not be in the patch.

~~~

ProcessPendingConflictLogTuple:

3.
+ /* Open conflict log table and insert the tuple */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);
+ Assert(CONFLICTS_LOGGED_TO_TABLE(dest));

Maybe here it's better to say Assert(conflictlogrel);

======
Kind Regards,
Peter Smith.
Fujitsu Australia