On Tue, Jan 27, 2026 at 9:24 AM Dilip Kumar <[email protected]> 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
