Re: Proposal: Conflict log history table for Logical Replication - Mailing list pgsql-hackers

Hi Dilip.

Here are a couple of review comments for v6-0001.

======
GENERAL.

1.
Firstly, here is one of my "what if" ideas...

The current patch is described as making a "structured, queryable
record of all logical replication conflicts".

What if we go bigger than that? What if this were made a more generic
"structured, queryable record of logical replication activity"?

AFAIK, there don't have to be too many logic changes to achieve this.
e.g. I'm imagining it is mostly:

* Rename the subscription parameter "conflict_log_table" to
"log_table" or similar.
* Remove/modify the "conflict_" name part from many of the variables
and function names.
* Add another 'type' column to the log table -- e.g. everything this
patch writes can be type="CONFL", or type='c', or whatever.
* Maybe tweak/add some of the other columns for more generic future use

Anyway, it might be worth considering this now, before everything
becomes set in stone with a conflict-only focus, making it too
difficult to add more potential/unknown log table enhancements later.

Thoughts?

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

2.
+#include "funcapi.h"
+#include "funcapi.h"

double include of the same header.

~~~

3.
+static Datum tuple_table_slot_to_ri_json_datum(EState *estate,
+    Relation localrel,
+    Oid replica_index,
+    TupleTableSlot *slot);
+
+static void insert_conflict_log(EState *estate, Relation rel,
+ TransactionId local_xid,
+ TimestampTz local_ts,
+ ConflictType conflict_type,
+ RepOriginId origin_id,
+ TupleTableSlot *searchslot,
+ TupleTableSlot *localslot,
+ TupleTableSlot *remoteslot);

There were no spaces between any of the other static declarations, so
why is this one different?

~~~

insert_conflict_log:

insert_conflict_log:

4.
+#define MAX_CONFLICT_ATTR_NUM 15
+ Datum values[MAX_CONFLICT_ATTR_NUM];
+ bool nulls[MAX_CONFLICT_ATTR_NUM];
+ Oid nspid;
+ Oid confliglogreid;
+ Relation conflictlogrel = NULL;
+ int attno;
+ int options = HEAP_INSERT_NO_LOGICAL;
+ char    *conflictlogtable;
+ char    *origin = NULL;
+ char    *remote_origin = NULL;
+ HeapTuple tup;

Typo: Oops. Looks like that typo originated from my previous review
comment, and you took it as-is.

/confliglogreid/confliglogrelid/

~~~

5.
+ if (searchslot != NULL && !TupIsNull(searchslot))
  {
- tableslot = table_slot_create(localrel, &estate->es_tupleTable);
- tableslot = ExecCopySlot(tableslot, slot);
+ Oid replica_index = GetRelationIdentityOrPK(rel);
+
+ /*
+ * If the table has a valid replica identity index, build the index
+ * json datum from key value. Otherwise, construct it from the complete
+ * tuple in REPLICA IDENTITY FULL cases.
+ */
+ if (OidIsValid(replica_index))
+ values[attno++] = tuple_table_slot_to_ri_json_datum(estate, rel,
+ replica_index,
+ searchslot);
+ else
+ values[attno++] = tuple_table_slot_to_json_datum(searchslot);
  }
+ else
+ nulls[attno++] = true;

- /*
- * Initialize ecxt_scantuple for potential use in FormIndexDatum when
- * index expressions are present.
- */
- GetPerTupleExprContext(estate)->ecxt_scantuple = tableslot;
+ if (localslot != NULL && !TupIsNull(localslot))
+ values[attno++] = tuple_table_slot_to_json_datum(localslot);
+ else
+ nulls[attno++] = true;

- /*
- * The values/nulls arrays passed to BuildIndexValueDescription should be
- * the results of FormIndexDatum, which are the "raw" input to the index
- * AM.
- */
- FormIndexDatum(BuildIndexInfo(indexDesc), tableslot, estate, values, isnull);
+ if (remoteslot != NULL && !TupIsNull(remoteslot))
+ values[attno++] = tuple_table_slot_to_json_datum(remoteslot);
+ else
+ nulls[attno++] = true;

AFAIK, the TupIsNull() already includes the NULL check anyway, so you
don't need to double up those. I saw at least 3 conditions above where
the code could be simpler. e.g.

BEFORE
+ if (remoteslot != NULL && !TupIsNull(remoteslot))

SUGGESTION
if (!TupIsNull(remoteslot))

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