Re: Proposal: Conflict log history table for Logical Replication

On Wed, Jun 3, 2026 at 10:11 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Here is the rebased version of the upgrade and \dRs patch which is
> present in v45-0003 and v45-0004. There is no change in v45-0001 and
> v45-0002, they are the same patch as in [1].

Please find a couple of minor comments for v45-001:

1) drop_sub_conflict_log_table()

+ conflictrelname = get_rel_name(subconflictlogrelid);
+ if (conflictrelname == NULL)
+ elog(ERROR, "cache lookup failed for index %u",
+ subconflictlogrelid);

Shouldn't the error say - "cache lookup failed for relation" instead
of "index"?

2) AlterSubscription()
+ if (IsSet(opts.specified_opts, SUBOPT_CONFLICT_LOG_DEST))
+ {
+ ConflictLogDest old_dest =
+ GetConflictLogDest(sub->conflictlogdest);
+
+ if (opts.conflictlogdest != old_dest)
+ {
+ bool update_relid;
+ Oid relid = InvalidOid;
+
+ values[Anum_pg_subscription_subconflictlogdest - 1] =
+ CStringGetTextDatum(ConflictLogDestNames[opts.conflictlogdest]);
+ replaces[Anum_pg_subscription_subconflictlogdest - 1] = true;
+
+ update_relid = alter_sub_conflictlogdestination(sub,
+ opts.conflictlogdest,
+ &relid);

Small optimization: we call GetConflictLogDest() twice in this flow,
once to compute old_dest and again inside
alter_sub_conflictlogdestination().
Could we pass old_dest to alter_sub_conflictlogdestination() instead?

--
Thanks,
Nisha