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.
>
> @vignesh C Your patch needs to be rebased.
>
Hi Dilip,

I started reviewing the patches.
Here are minor comments for 0001 patch:

1. If allow_system_table_mods=on we can add/drop columns of conflict log tables
But the same for pg_toast or other catalog tables are prohibited. Also
for other system tables we are getting following error.

postgres=# ALTER TABLE pg_toast.pg_toast_16413 DROP COLUMN chunk_seq;
ERROR: ALTER action DROP COLUMN cannot be performed on relation
"pg_toast_16413"

DETAIL: This operation is not supported for TOAST tables.
postgres=# ALTER TABLE pg_publication DROP COLUMN pubname;
ERROR: cannot drop column pubname of table pg_publication because it
is required by the database system
postgres=# ALTER TABLE pg_description DROP COLUMN description;
ERROR: cannot drop column description of table pg_description because
it is required by the database system

postgres=# ALTER TABLE pg_conflict.pg_conflict_log_16408 DROP COLUMN relname;
ALTER TABLE

Should we prohibit it for conflict log tables as well?

2. Should we also have a 'dropped conflict log table' NOTICE, when the
subscription is dropped?
postgres=# CREATE SUBSCRIPTION sub1 connection 'dbname=postgres
host=localhost port=5432' publication pub1 WITH
(conflict_log_destination = 'TABLE');
NOTICE: created conflict log table
"pg_conflict.pg_conflict_log_16394" for subscription "sub1"
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION
postgres=# drop subscription sub1;
NOTICE: dropped replication slot "sub1" on publisher
DROP SUBSCRIPTION

3. Typo:
+ /*
+ * Check for an existing table with the sname name in the
pg_conflict namespace.

sname -> same

Thanks,
Shlok Kyal