Thread

Re: Proposal: Conflict log history table for Logical Replication

shveta malik <shveta.malik@gmail.com> — 2026-05-14T08:52:11Z

On Mon, May 11, 2026 at 4:14 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 8 May 2026 at 17:40, 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.
>
> 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