Re: Proposal: Conflict log history table for Logical Replication

On Wed, 13 May 2026 at 11:43, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Dilip/Vignesh.
>
> Some review comments for v33-0001.
>
> ======
> src/backend/executor/execMain.c
>
> 11.
> +
> + /*
> + * Conflict log tables are managed by the system to record logical
> + * replication conflicts.  We allow DELETE and TRUNCATE to permit users to
> + * manually prune these logs, but manual data insertion or modification
> + * (INSERT, UPDATE, MERGE) is prohibited to maintain the integrity of the
> + * system-generated logs.
> + *
> + * Since TRUNCATE is handled as a separate utility command, we only need
> + * to explicitly permit CMD_DELETE here.
> + */
> + if (IsConflictNamespace(RelationGetNamespace(resultRel)) &&
> + operation != CMD_DELETE)
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("cannot modify or insert data into conflict log table \"%s\"",
> + RelationGetRelationName(resultRel)),
> + errdetail("Conflict log tables are system-managed and only support
> cleanup via DELETE or TRUNCATE.")));
>
> It somehow feels backwards to check "operation != CMD_DELETE", with
> the obscure comment that TRUNCATE is handled elsewhere.
>
> How about just check if "(operation == CMD_INSERT || operation ==
> CMD_UPDATE || operation == CMD_MERGE)".

I felt the existing is ok here, as it is mentioned "we only need to
explicitly permit CMD_DELETE" . Are you seeing any commands other than
INSERT, UPDATE & MERGE possible here?

> ~~~
>
> 12.
> +
> + /*
> + * 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 log table \"%s\"",
> + RelationGetRelationName(rel))));
>
> I was not sure what was meant by "CONFLICT relations.".
>
> Does it mean "... relations in the pg_conflict schema.". Anyway, is
> there any value to that 2nd sentence because it is much the same text
> as the errmsg.

 Yes, it means the relations in pg_conflict schema. Removed the second sentence.

> ======
> src/backend/replication/logical/conflict.c
>
> 13.
> +const char *const ConflictLogDestNames[] = {
> + [CONFLICT_LOG_DEST_LOG] = "log",
> + [CONFLICT_LOG_DEST_TABLE] = "table",
> + [CONFLICT_LOG_DEST_ALL] = "all"
> +};
> +
> +const ConflictLogColumnDef v[] = {
> + { .attname = "relid",            .atttypid = OIDOID },
> + { .attname = "schemaname",       .atttypid = TEXTOID },
> + { .attname = "relname",          .atttypid = TEXTOID },
> + { .attname = "conflict_type",    .atttypid = TEXTOID },
> + { .attname = "remote_xid",       .atttypid = XIDOID },
> + { .attname = "remote_commit_lsn",.atttypid = LSNOID },
> + { .attname = "remote_commit_ts", .atttypid = TIMESTAMPTZOID },
> + { .attname = "remote_origin",    .atttypid = TEXTOID },
> + { .attname = "replica_identity", .atttypid = JSONOID },
> + { .attname = "remote_tuple",     .atttypid = JSONOID },
> + { .attname = "local_conflicts",  .atttypid = JSONARRAYOID }
> +};
>
> 13a.
> Both these arrays could benefit with some comments.

Added comments

> ~
>
> 13b.
> In the ConflictLogSchema, would it be better to keep all those
> "remote_" columns grouped together, instead of being broken by
> "replica_identity".

Modified

> ~
>
> 13c.
> TBH, I preferred code how it used to be -- where all the CLT constants
> and structs and enums and schemas were kept together. Now they are
> split across conflict.h and conflict.c making it harder to read as
> well as introducing need for static asserts that were not needed
> before.

No change done, as this change is required. Amit has given the
explanation at [1].

Rest of the comments were addressed. The attached v35 version patch
has the changes for the same.

I have kept the review comment fixes as separate patches so that Dilip
can merge them when convenient. Due to the additional review-fix
patches, Dilip's original patches 0001, 0002, 0003, and 0004 are now
renumbered as 0001, 0003, 0005, and 0007 respectively.  The
intermediate patches contain the review comment fixes:
a) 0002 contains fixes for 0001 b) 0004 contains fixes for 0003 c)
0006 contains fixes for 0005 d) 0008 contains fixes for 0007

Also comments from [2] and [3] are addressed in this.

[1] - https://www.postgresql.org/message-id/CAA4eK1Ki5mBgAubBkUPcBjN%3DO1jeT3AUh7vLQBm8w%3DgQiHO5Jw%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHut%2BPv%2BBK7iM3KZNcrXzPMYagrL2O4%3D46Hi3stT2XT-RmsjRQ%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CAJpy0uARoVZkTA_PV4PB1MtUXZMyxkun1Cg5o1YOxaKsCbWxCA%40mail.gmail.com

Regards,
Vignesh