On Mon, May 11, 2026 at 4:14 PM vignesh C <[email protected]> wrote:
>
> On Fri, 8 May 2026 at 17:40, Dilip Kumar <[email protected]> wrote:
> >
> > On Fri, May 8, 2026 at 8:28 AM Dilip Kumar <[email protected]> wrote:
> > >
> > > On Thu, May 7, 2026 at 5:24 PM Nisha Moond <[email protected]>
> > > 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.
>
Thanks Vignesh, Please find a few comments on 0005:
1)
listSubscriptions has:
+ pg_log_error("The server (version %s) does not support publications.",
publications --> subscriptions
2)
printfPQExpBuffer(&buf, "/* %s */\n", _("Get matching subscriptions"));
appendPQExpBuffer(&buf,
"SELECT subname AS \"%s\"\n"
", pg_catalog.pg_get_userbyid(subowner) AS \"%s\"\n"
", subenabled AS \"%s\"\n"
", subpublications AS \"%s\"\n",
gettext_noop("Name"),
gettext_noop("Owner"),
gettext_noop("Enabled"),
gettext_noop("Publication"));
/* Only display subscriptions in current database. */
appendPQExpBufferStr(&buf,
"FROM pg_catalog.pg_subscription\n"
"WHERE subdbid = (SELECT oid\n"
" FROM pg_catalog.pg_database\n"
" WHERE datname = pg_catalog.current_database())");
Why have we split the query? Can we have it in one go itself?
3)
+ appendPQExpBuffer(&buf,
+ "SELECT oid, subname AS \"%s\"\n"
+ ", pg_catalog.pg_get_userbyid(subowner) AS \"%s\"\n"
+ ", subenabled AS \"%s\"\n"
+ ", subpublications AS \"%s\"\n",
+ gettext_noop("Name"),
+ gettext_noop("Owner"),
+ gettext_noop("Enabled"),
+ gettext_noop("Publication"));
+ ncols = 3;
The query has 5 columns and we have set ncols as 3. A comment will help here.
4)
+ snprintf(conflictlogtable,
+ sizeof(conflictlogtable),
+ "pg_conflict.pg_conflict_log_%s",
+ subid);
Should be avoid hard-coding the namespace name like above?
thanks
Shveta
