Re: Proposal: Conflict log history table for Logical Replication

Hi Dilip.

Some comments for the first 2 patches:

//////////
v24-00001
//////////

1.
+ /*
+ * Conflict log tables are managed by the system to record logical
+ * replication conflicts. We do not allow locking rows in CONFLICT
+ * relations.
+ */
+ if (IsConflictNamespace(RelationGetNamespace(rel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot lock rows in CONFLICT relation \"%s\"",
+ RelationGetRelationName(rel))));

AFAIK, this "CONFLICT relation" terminology is not used anywhere else.

Why not just call it what it is:

e.g.
cannot lock rows in conflict log table \"%s\"

~

OTOH, if you were attempting to future-proof the message for different
kinds of relations in the 'pg_conflict' namespace, I still felt it
might be better to refer to 'pg_conflict' instead of CONFLICT:

e.g.
cannot lock rows in 'pg_conflict' relation \"%s\"

//////////
v24-0002
//////////

1.
+static char *build_index_value_desc(EState *estate, Relation localrel,
+ TupleTableSlot *slot, Oid indexoid);

Declared twice?

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