Hi. Some review comments for v9-0001.
======
Commit message.
1.
Note: A single remote tuple may conflict with multiple local conflict
when conflict type
is CT_MULTIPLE_UNIQUE_CONFLICTS, so for handling this case we create a
single row in
conflict log table with respect to each remote conflict row even if it
conflicts with
multiple local rows and we store the multiple conflict tuples as a
single JSON array
element in format as
[ { "xid": "1001", "commit_ts": "...", "origin": "...", "tuple": {...} }, ... ]
We can extract the elements from local tuple as given in below example
~
Something seems broken/confused with this description:
1a.
"A single remote tuple may conflict with multiple local conflict"
Should that say "... with multiple local tuples" ?
~
1b.
There is a mixture of terminology here, "row" vs "tuple", which
doesn't seem correct.
~
1c.
"We can extract the elements from local tuple"
Should that say "... elements of the local tuples from the CLT row ..."
======
src/backend/replication/logical/conflict.c
2.
+
+#define N_LOCAL_CONFLICT_INFO_ATTRS 4
I felt it would be better to put this where it is used. e.g. IMO put
it within the build_conflict_tupledesc().
~~~
InsertConflictLogTuple:
3.
+ /* A valid tuple must be prepared and store in MyLogicalRepWorker. */
Typo still here: /store in/stored in/
~~~
4.
+static TupleDesc
+build_conflict_tupledesc(void)
+{
+ TupleDesc tupdesc;
+
+ tupdesc = CreateTemplateTupleDesc(N_LOCAL_CONFLICT_INFO_ATTRS);
+
+ TupleDescInitEntry(tupdesc, (AttrNumber) 1, "xid",
+ XIDOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 2, "commit_ts",
+ TIMESTAMPTZOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 3, "origin",
+ TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 4, "tuple",
+ JSONOID, -1, 0);
If you had some incrementing attno instead of hard-wiring the
(1,2,3,4) then you'd be able to add a sanity check like Assert(attno +
1 == N_LOCAL_CONFLICT_INFO_ATTRS); that can safeguard against future
mistakes in case something changes without updating the constant.
~~~
build_local_conflicts_json_array:
5.
+ /* Process local conflict tuple list and prepare a array of JSON. */
+ foreach(lc, conflicttuples)
{
- tableslot = table_slot_create(localrel, &estate->es_tupleTable);
- tableslot = ExecCopySlot(tableslot, slot);
+ ConflictTupleInfo *conflicttuple = (ConflictTupleInfo *) lfirst(lc);
5a.
typo in comment: /a array/an array/
~
5b.
SUGGESTION
foreach_ptr(ConflictTupleInfo, conflicttuple, confrlicttuples)
{
~~~
6.
+ i = 0;
+ foreach(lc, json_datums)
+ {
+ json_datum_array[i] = (Datum) lfirst(lc);
+ json_null_array[i] = false;
+ i++;
+ }
6a.
The loop seemed to be unnecessarily complicated since you already know
the size. Isn't it the same as below?
SUGGESTION
for (int i = 0; i < num_conflicts; i++)
{
json_datum_array[i] = (Datum) list_nth(json_datums, i);
json_null_array[i] = false;
}
6b.
Also, there is probably no need to do json_null_array[i] = false; at
every iteration here, because you could have just used palloc0 for the
whole array in the first place.
======
src/test/regress/expected/subscription.out
7.
+-- check if the table exists and has the correct schema (15 columns)
+SELECT count(*) FROM pg_attribute WHERE attrelid =
'public.regress_conflict_log1'::regclass AND attnum > 0;
+ count
+-------
+ 11
+(1 row)
+
That comment is wrong; there aren't 15 columns anymore.
~~~
8.
(mentioned in a previous review)
I felt that \dRs should display the CLT's schema name in the "Conflict
log table" field -- at least when it's not "public". Otherwise, it
won't be easy for the user to know it.
I did not see a test case for this.
~~~
9.
(mentioned in a previous review)
You could have another test case to explicitly call the function
pg_relation_is_publishable(clt) to verify it returns false for a CTL
table.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
