Re: Proposal: Conflict log history table for Logical Replication

On Tue, Jan 27, 2026 at 8:39 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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\"

I prefer conflict log tables for consistency with other places.

> //////////
> v24-0002
> //////////
>
> 1.
> +static char *build_index_value_desc(EState *estate, Relation localrel,
> + TupleTableSlot *slot, Oid indexoid);
>
> Declared twice?

Removed duplicate.

Also fixed all pending doc comments.



-- 
Regards,
Dilip Kumar
Google