Re: Proposal: Conflict log history table for Logical Replication

On Thu, May 14, 2026 at 12:45 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> I have fixed the other comments except this one. I will think more
> about this and reply separately. The attached patch has the changes
> for the rest of the comments. The patch also addresses comments from
> [1].
>

Thanks for the patches. Please find below comments for v34 patch-set.

1) Bug report:
When disable_on_error = true for a subscription, and an ERROR-level
conflict such as insert_exists occurs, the subscription gets disabled
without logging the conflict into the CLT.

patch-001:
2) execMain.c:
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("cannot modify or insert data into conflict log table \"%s\"",
+ RelationGetRelationName(resultRel)),

Is ERRCODE_INSUFFICIENT_PRIVILEGE the right error code here? It gives
the impression that the operation might succeed with higher
privileges. Should we instead use ERRCODE_WRONG_OBJECT_TYPE, similar
to nearby restrictions?

3) No notice is shown when the conflict log table is removed after
changing conflict_log_destination from table/all to log.
Example:
postgres=# alter subscription sub1 set (conflict_log_destination = table);
NOTICE: created conflict log table
"pg_conflict.pg_conflict_log_16400" for subscription "sub1"
ALTER SUBSCRIPTION

postgres=# alter subscription sub1 set (conflict_log_destination = log);
ALTER SUBSCRIPTION

We already show a notice when changing from log to table/all. Should
we add a similar notice as in DROP SUBSCRIPTION for above case?

patch-003:
4) conflict.c: ReportApplyConflict()
+ bool log_dest_clt = false;
+ bool log_dest_logfile;

log_dest_logfile should also be initialized to false, since for dest
== CONFLICT_LOG_DEST_TABLE, it is never assigned.

5) worker_internal.h
extern PGDLLIMPORT List *table_states_not_ready;

+extern XLogRecPtr remote_final_lsn;
+extern TimestampTz remote_commit_ts;
+extern TransactionId remote_xid;

Should these new declarations also use PGDLLIMPORT?

6) worker.c: apply_handle_stream_start()
+ remote_xid = stream_xid;
+ remote_final_lsn = InvalidXLogRecPtr;
+ remote_commit_ts = 0;
+
if (!TransactionIdIsValid(stream_xid))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),

Should the remote_xid assignment be moved after the validity check? We
could move all three assignments below the check.

patch-005:
7) subscriptioncmds.c: DropSubscription()
+ if (OidIsValid(form->subconflictlogrelid))
+ {
+ char *conflictrelname = get_rel_name(form->subconflictlogrelid);
....
"form" is being used here after the tuple it points to has already been deleted:

/* Remove the tuple from catalog. */
CatalogTupleDelete(rel, &tup->t_self);

ReleaseSysCache(tup);

I think form->subconflictlogrelid should be saved beforehand and then
used later, similar to subid.

--
Thanks,
Nisha