Re: Proposal: Conflict log history table for Logical Replication

On Fri, 8 May 2026 at 17:40, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Fri, May 8, 2026 at 8:28 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Thu, May 7, 2026 at 5:24 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)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.

The attached patch addresses the remaining open items and is provided
separately as patch 0005. @Dilip Kumar, if the changes look good to
you, please merge them into the corresponding patch.

Regards,
Vignesh