Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jan 27, 2026 at 9:24 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
>
> Also fixed all pending doc comments.
>

Thanks for the patch, Please find a few comments:

patch0001:

1)
+/*
+ * GetLogDestination
+ *
+ * Convert string to enum by comparing against standardized labels.
+ */
+ConflictLogDest
+GetLogDestination(const char *dest)

IMO the name of the function should be GetConflictLogDest, otherwise
the current name might suggest it applies to all the logs related to
subscription, which is misleading.

2)
+ { .attname = "schemaname",       .atttypid = TEXTOID },

I checked the catalog tables known to me which refer to namespace, all
use namespace or nsp in their names, none use 'schema'. Examples:
pg_namespace, pg_class, pg_type. pg_constraint.  Shall we use nspname
instead?

<No issues found in my local testing for 0001>

~~

patch0002:

1)
conflict.c compiles without these:

+#include "utils/fmgroids.h"
+#include "utils/json.h"

2)
+ /*
+ * Get the conflict log destination. Also, (if there is one) return the
+ * CLT relation already opened and ready for insertion.
+ */
+ conflictlogrel = GetConflictLogDestAndTable(&dest);

'CLT relation' means conflict log table relation. Shall we correct it
to mention either table alone or relation alone?

3)
This is defined in conflict.h:

+/* The single source of truth for the conflict log table schema */
+static const ConflictLogColumnDef ConflictLogSchema[] =
+{
....
+ { .attname = "local_conflicts",  .atttypid = JSONARRAYOID }
+};

while its element 'local_conflicts' is defined in conflict.c:

+/* Schema for the elements within the 'local_conflicts' JSON array */
+static const ConflictLogColumnDef LocalConflictSchema[] =
+{
...
+};

It takes some time to figure this part out as a reader of code. I
think we shall define LocalConflictSchema schema immediately after
ConflictLogSchema for anyone to understand it better, unless there is
something blocking it?

4)
+# Verify that '2' is present inside the JSON structure using a regex
+# This matches the key/value pattern for "a": 2
+like($raw_json, qr/\\"a\\":2/, 'Verified that key 2 exists in the
local_conflicts');
+

Reviewing further. Testing of 0002 yet to be finished.

thanks
Shveta