Re: Proposal: Conflict log history table for Logical Replication

A few comments:

1)
+ or both. Note that only resolved conflicts (logged at
<literal>LOG</literal>
+ level where the apply transaction continues) are recorded in the
conflict log
+ table. Conflicts that raise an <literal>ERROR</literal> halt
replication and
+ are always reported exclusively to the server log.

I think this is not needed in the Conflict-logging introduction
section itself. It is more suited in 'table-logging' section. We can
keep it as a NOTE (<note> tag) so that it catches reader’s eye. We can
add it before 'The conflict log table serves as an audit trail'
paragraph.

2)
+conflict_log_insert_errcontext(void *arg)

I think we don't need this callbackfunction and logic around it anymore.

3)
In tuple_table_slot_to_indextup_json(), we first call
BlessTupleDesc(tupdesc), but then free the tuple descriptor with
FreeTupleDesc(tupdesc) before calling row_to_json().

I am not entirely sure whether row_to_json() may need the tuple
descriptor here, but it seems safer to keep the descriptor alive until
after row_to_json() has consumed the record Datum. heap_form_tuple()
and heap_copy_tuple_as_datum() also use the tuple descriptor while
constructing the Datum;see heap_fill_tuple() uses the tuple
descriptor's attributes. IMO, to be on the safe side, we could do:

/* Convert to a JSON datum. */
datum = DirectFunctionCall1(row_to_json, datum);
FreeTupleDesc(tupdesc);
return datum;

thanks
Shveta