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.
--
Regards,
Dilip Kumar
Google
