Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip. Some review comments for v7-0001.

======
src/backend/replication/logical/conflict.c

1.
+ /* Insert conflict details to conflict log table. */
+ if (conflictlogrel)
+ {
+ /*
+ * Prepare the conflict log tuple. If the error level is below
+ * ERROR, insert it immediately. Otherwise, defer the insertion to
+ * a new transaction after the current one aborts, ensuring the log
+ * tuple is not rolled back.
+ */
+ conflictlogtuple = prepare_conflict_log_tuple(estate,
+ relinfo->ri_RelationDesc,
+ conflictlogrel,
+ conflicttuple->xmin,
+ conflicttuple->ts, type,
+ conflicttuple->origin,
+ searchslot, conflicttuple->slot,
+ remoteslot);
+ if (elevel < ERROR)
+ {
+ InsertConflictLogTuple(conflictlogrel, conflictlogtuple);
+ heap_freetuple(conflictlogtuple);
+ }
+ else
+ MyLogicalRepWorker->conflict_log_tuple = conflictlogtuple;
+
+ table_close(conflictlogrel, AccessExclusiveLock);
+ }
+ }
+

IMO, some refactoring would help simplify conflictlogtuple processing. e.g.

i) You don't need any separate 'conflictlogtuple' var
- Use MyLogicalRepWorker->conflict_log_tuple always for this purpose
ii) prepare_conflict_log_tuple()
- Change this to a void; it will always side-effect
MyLogicalRepWorker->conflict_log_tuple
- Assert MyLogicalRepWorker->conflict_log_tuple must be NULL on entry
iii) InsertConflictLogTuple()
- The 2nd param it not needed if you always use
MyLogicalRepWorker->conflict_log_tuple
- Asserts MyLogicalRepWorker->conflict_log_tuple is not NULL, then writes it
- BTW, I felt that heap_freetuple could also be done here too
- Finally, sets to MyLogicalRepWorker->conflict_log_tuple to NULL
(ready for the next conflict)

~~~

InsertConflictLogTuple:

2.
+/*
+ * InsertConflictLogTuple
+ *
+ * Persistently records the input conflict log tuple into the conflict log
+ * table. It uses HEAP_INSERT_NO_LOGICAL to explicitly block logical decoding
+ * of the tuple inserted into the conflict log table.
+ */
+void
+InsertConflictLogTuple(Relation conflictlogrel, HeapTuple tup)
+{
+ int options = HEAP_INSERT_NO_LOGICAL;
+
+ heap_insert(conflictlogrel, tup, GetCurrentCommandId(true), options, NULL);
+}

See the above review comment (iii), for some suggested changes to this function.

~~~

prepare_conflict_log_tuple:

3.
+ * The caller is responsible for explicitly freeing the returned heap tuple
+ * after inserting.
+ */
+static HeapTuple
+prepare_conflict_log_tuple(EState *estate, Relation rel,

As per the above review comment (iii), I thought the Insert function
could handle the freeing.

~~~

4.
+ oldctx = MemoryContextSwitchTo(ApplyContext);
+ tup = heap_form_tuple(RelationGetDescr(conflictlogrel), values, nulls);
+ MemoryContextSwitchTo(oldctx);

- return index_value;
+ return tup;

Per the above comment (ii), change this to assign to
MyLogicalRepWorker->conflict_log_tuple.

======
src/backend/replication/logical/worker.c

start_apply:

5.
+ /*
+ * Insert the pending conflict log tuple under a new transaction.
+ */

/Insert the/Insert any/

~~~

6.
+ InsertConflictLogTuple(conflictlogrel,
+ MyLogicalRepWorker->conflict_log_tuple);
+ heap_freetuple(MyLogicalRepWorker->conflict_log_tuple);
+ MyLogicalRepWorker->conflict_log_tuple = NULL;

Per earlier reqview comment (iii), remove the 2nd parm to
InsertConflictLogTuple, and those other 2 statements can also be
handled within InsertConflictLogTuple.

======
src/include/replication/worker_internal.h

7.
+ /* Store conflict log tuple to be inserted before worker exit. */
+ HeapTuple conflict_log_tuple;
+

Per my above suggestions, this member comment becomes something more
like "A conflict log tuple which is prepared but not yet written. */

======
Kind Regards,
Peter Smith.
Fujitsu Australia