Hi Dilip.
Here are some review comments for the v19-0002 (code only, not tests).
======
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 }
+};
Maybe this only needs to be used in this module, but I still thought
LocalConflictSchema[] (and the MAX_LOCAL_CONFLICT_INFO_ATTRS) belongs
more naturally alongside the other ConflictLogSchema[] (e.g. in
conflict.h)
~~~
2.
+
+#define MAX_LOCAL_CONFLICT_INFO_ATTRS \
+ (sizeof(LocalConflictSchema) / sizeof(LocalConflictSchema[0]))
+
how about:
#define MAX_LOCAL_CONFLICT_INFO_ATTRS lengthof(LocalConflictSchema)
~~~
ReportApplyConflict:
3.
+ /* Insert to table if destination is 'table' or 'all' */
+ if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
I think it is unnecessary to even mention 'all'. The bitmasks tell
everything you need to know.
SUGGESTION
Insert to table if requested.
~
4.
There's a slightly convoluted if;if/else with these bitmasks here, but
I think Shveta already suggested the same change as what I was
thinking.
~~~
GetConflictLogTableInfo:
5.
+ *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;
I think checking and mentioning 'log' here is not future-proof for
when there might be more kinds of destinations. We just want to return
when the user doesn't want a 'table'. Use the bitmasks to do that.
SUGGESTION
/* Quick exit if a conflict log table was not requested. */
if ((*logdest & CONFLICT_LOG_DEST_TABLE) == 0)
return NULL;
~~~
build_conflict_tupledesc:
6.
+static TupleDesc
+build_conflict_tupledesc(void)
+{
Missing function comment. There used to be one (??).
~~~
build_local_conflicts_json_array:
7.
+ json_datum_array = (Datum *) palloc(num_conflicts * sizeof(Datum));
+ json_null_array = (bool *) palloc0(num_conflicts * sizeof(bool));
I may have already asked this same question in a previous review, but
shouldn't these be using those new palloc_array and palloc0_array
macros?
======
src/include/replication/conflict.h
8.
-/* Define the count using the array size */
#define MAX_CONFLICT_ATTR_NUM (sizeof(ConflictLogSchema) /
sizeof(ConflictLogSchema[0]))
This change appears to be in the wrong patch. AFAICT it belonged in 0001.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
