Re: Proposal: Conflict log history table for Logical Replication

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