Re: Proposal: Conflict log history table for Logical Replication

On Mon, May 11, 2026 at 2:59 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> 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?
>

Good catch Shlok, yes it should be restricted IMO.

Another thing I found was that we could attach CLT as a partition of
another table. And then add it indirectly to publication.

Test:
-------------------------
CREATE TABLE public.conflict_parent (LIKE
pg_conflict.pg_conflict_log_16387 INCLUDING ALL) PARTITION BY LIST
(conflict_type);

ALTER TABLE public.conflict_parent ATTACH PARTITION
pg_conflict.pg_conflict_log_16387 FOR VALUES IN ('insert_exists');

CREATE publication pub1 FOR TABLE public.conflict_parent
WITH(PUBLISH_VIA_PARTITION_ROOT =false);

postgres=# select * from pg_publication_tables;
pubname | schemaname | tablename
---------+-------------+-----------------------+------------
pub1 | pg_conflict | pg_conflict_log_16387

---------------------------

While for toast table, 'LIKE' operation failed for the toast table:

postgres=# CREATE TABLE public.fake_toast_parent ( LIKE
pg_toast.pg_toast_16459 INCLUDING ALL) PARTITION BY LIST (chunk_seq);
ERROR: relation "pg_toast_16459" is invalid in LIKE clause
LINE 1: CREATE TABLE public.fake_toast_parent ( LIKE pg_toast.pg_toa...

^
DETAIL: This operation is not supported for TOAST tables.

~~

Trying it differently, attaching it as a partition also fails.

postgres=# CREATE TABLE public.fake_toast_parent ( chunk_id oid,
chunk_seq int4, chunk_data bytea) PARTITION BY LIST (chunk_seq);
CREATE TABLE
postgres=# ALTER TABLE public.fake_toast_parent ATTACH PARTITION
pg_toast.pg_toast_16459 FOR VALUES IN (1);
ERROR: ALTER action ATTACH PARTITION cannot be performed on relation
"pg_toast_16459"
DETAIL: This operation is not supported for TOAST tables.

~~

I have tried above tests with allow_system_table_mods=on;

So toast table does not support 'LIKE'.
It also does not support attaching it as a partition to another table.

IMO, we need the same restrcitions for CLT. Thoughts?

thanks
Shveta