Re: Proposal: Conflict log history table for Logical Replication

On Mon, 11 May 2026 at 11:51, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, May 8, 2026 at 5:40 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, May 8, 2026 at 8:28 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Thu, May 7, 2026 at 5:24 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > > >
> > > > > The attached v31 version has the changes to fix this issue by
> > > > > initializing the variable.
> > > > > This also has the rebased version along with the rebased version of
> > > > > the 'Preserve conflict log destination and subscription OID for
> > > > > subscriptions' patch which is present in the 0005 patch.
> > > >
> > > > Thanks for the patches, please find a few comments on the patches 002 to 004:
> > > >
> > > > 1) I noticed that if a non-superuser creates the subscription, but a
> > > > superuser later runs:
> > > >   ALTER SUBSCRIPTION ... SET (conflict_log_table = all)
> > > > then the conflict table ends up being owned by the superuser instead
> > > > of the subscription owner. Though, apply_worker would be able to
> > > > insert into the CLT, but the subscription owner cannot access its
> > > > associated conflict log table,
> > > >
> > > > I think this happens because the heap_create_with_catalog() call uses
> > > > GetUserId(). It is fine during CREATE SUBSCRIPTION, but during ALTER
> > > > SUBSCRIPTION, it causes the table to be created under the ALTER
> > > > command executor’s ownership instead of the subscription owner.
> > > >
> > > > Since only the subscription owner or a superuser can run ALTER
> > > > SUBSCRIPTION, should we always create the table with the subscription
> > > > owner as the owner?
> > >
> > > Yeah that makes sense.
> > >
> > > > 2) In GetConflictLogDestAndTable():
> > > > + conflictlogrel = table_open(conflictlogrelid, RowExclusiveLock);
> > > > + if (conflictlogrel == NULL)
> > > > + elog(ERROR, "could not open conflict log table (OID %u)",
> > > > + conflictlogrelid);
> > > > +
> > > > + return conflictlogrel;
> > > >
> > > > I think the "if (conflictlogrel == NULL)" check is unreachable. The
> > > > table_open()->relation_open() will error-out if it fails to open the
> > > > relation.
> > >
> > > Yeah, that's a valid point.
> > >
> > > > 3) Minor typo in create_conflict_log_table() comments:
> > > > + /*
> > > > + * Check for an existing table with the sname name in the pg_conflict
> > > > namespace.
> > > > + * A collision  should not occur under normal operation, but we must
> > > > handle cases
> > > > + * where a table has been created manually.
> > > > + */
> > > > ==> double space in "A collision  should not"
> > > >
> > > > 4) The document patch-0004 is still referring to the old name
> > > > "pg_conflict_<subid>", it should be "pg_conflict_log_<subid>".
> > >
> > > I will fix these in next version.
> > >
> >
> > This fixes all 4 comments Nisha reported.  And 0002 is an add-on patch
> > to allow ownership transfer.  I haven't yet changed the clt display
> > witjh \dRs+ reported by shveta.  I have a work-in-progress patch, but
> > I couldn't get it to work.  I will try to debug that tomorrow or next
> > week whenever I get time.
> >
> > Open Items:
> > -  Add comments explaining the reasoning for the ownership change
> > - change clt display
> > - Test cases for ownership change, truncation, deletion, and select
> > from a non-superuser owner of subscriber.
> >
> > @vignesh C Your patch needs to be rebased.
> >
>
> Few comments on 001:
>
> 3)
> Currently the structure of CLT is:
>
> +const ConflictLogColumnDef ConflictLogSchema[] = {
> + { .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 }
> +};
>
> So if user has to delete a conflict from CLT after resolving it, then
> what is the user-friendly way to do it? IMO, it will be cumbersome
> (and perhaps error-prone) to write a query with remote_commit_lsn,
> remote_commit_ts, remote_xid etc in WHERE clause. Do you (or others)
> think we shall add a log_id column (perhaps a bigint GENERATED ALWAYS
> AS IDENTITY). This provides a simple, unique identifier so the user
> can easily target a single row (WHERE log_id = 105) or purge a batch
> of old conflicts (WHERE log_id < 1000).

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].

[1] - https://www.postgresql.org/message-id/CAJpy0uANkzTyUjO2W0%3DRtaJCGg%3DVYcwLGGCpqax%3DzKJgNbB0Hw%40mail.gmail.com

Regards,
Vignesh