On Mon, Jun 8, 2026 at 7:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Jun 8, 2026 at 11:43 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Jun 5, 2026 at 4:22 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > >
> > > On Fri, Jun 5, 2026 at 3:06 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > On Fri, Jun 5, 2026 at 11:53 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > >
> > > > > On Thu, Jun 4, 2026 at 4:05 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > > > >
> > > > > > I noticed that it is currently possible to acquire explicit locks on a CLT:
> > > > > >
> > > > > > --Session locks table and does not commit txn:
> > > > > > postgres=# BEGIN;
> > > > > > LOCK TABLE pg_conflict.pg_conflict_log_16481 IN SHARE MODE;
> > > > > > BEGIN
> > > > > > LOCK TABLE
> > > > > >
> > > > > > Doing so can cause the apply worker to block indefinitely when it
> > > > > > attempts to modify the CLT:
> > > > > >
> > > > > > [247433] LOG:  logical replication apply worker for subscription
> > > > > > "sub1" has started
> > > > > > [247433] LOG:  process 247433 still waiting for RowExclusiveLock on
> > > > > > relation 16482 of database 5 after 1001.030 ms
> > > > > > [247433] DETAIL:  Process holding the lock: 245584. Wait queue: 247433.
> > > > > > [247433] CONTEXT:  waiting for RowExclusiveLock on relation 16482 of database 5
> > > > > >
> > > > > > Toast Table behaviour:
> > > > > > postgres=*# LOCK TABLE pg_toast.pg_toast_16384 IN SHARE MODE;
> > > > > > ERROR:  cannot lock relation "pg_toast_16384"
> > > > > > DETAIL:  This operation is not supported for TOAST tables.
> > > > > >
> > > > > > Should we consider disallowing explicit LOCK TABLE operations on CLTs,
> > > > > > similar to how PostgreSQL handles TOAST tables? Or does anyone see any
> > > > > > legitimate use-case (I don't) where we would need to allow explicit
> > > > > > LOCKs on CLT?
> > > > >
> > > > > We need to add namespace-based checks here, as the current logic
> > > > > relies solely on relkind [1], which classifies TOAST tables
> > > > > separately. In my view, choosing to either allow or disallow this
> > > > > behavior will not cause significant inconvenience or seem unusual to
> > > > > anyone. Therefore, I prefer the path that minimizes special-purpose
> > > > > code. Since explicitly disallowing this requires additional
> > > > > special-purpose logic (as shown below [1]), allowing it seems to be
> > > > > the cleaner approach.  Thoughts?
> > > >
> > > > Okay, upon analyzing this new logic,  I too prefer to allow it.
> > > >
> > > > I was thinking if there is a way to set lock_timeout in
> > > > ProcessPendingConflictLogTuple() or try to acquire lock and if it
> > > > fails we hit 'ERRCODE_LOCK_NOT_AVAILABLE', log a different warning in
> > > > the log file and let the apply worker proceed.
> > > >
> > > > But if this too is complicated, I am fine with the current
> > > > implementation. Since LOCK TABLE is a well-known command, if a user
> > > > explicitly locks a CLT, they should be responsible for the
> > > > consequences such as blocking the apply worker.
> > >
> > > +1
> > >
> > > Here is the updated patch which fixes all open issues except Peter
> > > reported on 0004 patch, Vignesh would you take care of that?
> > >
> >
> > Thank You Dilip.
> >
> > v46-001:
> >
> > 1)
> >
> > +static bool alter_sub_conflictlogdestination(Subscription *sub,
> > + ConflictLogDest oldlogdest,
> > + ConflictLogDest newlogdest,
> > + Oid *conflicttablerelid);
> >
> > +static void drop_sub_conflict_log_table(Oid subid, char *subname,
> > +
> >
> > Can we name alter_sub_conflictlogdestination to
> > alter_sub_conflict_log_dest?  Feel free to ignore if you find current
> > name better.
>
> Yeah we may change that.
>
> > 2)
> > Ran all the tests again on 0001 alone, inheritance is still working.
> > Let me know if we decided to retain it.  IMO, it is better to block it
> > for the reasons stated earlier.  The changes can be made in
> > MergeAttributes and ATExecAddInherit; we already have similar relation
> > based restrictions there, one more can be added for CLT.
> >
> > ~~
> >
> > No major issue in 0001, it seems be in good shape. Will do one more
> > round of reveiw and testing on next version though.
>
> After rethinking my previous stance on blocking these operations, let
> me clarify the core principle I think we should follow for CLTs. I am
> completely open to feedback on this approach:
>
> 1. Block Direct Mutations: We should block any operation that directly
> modifies the CLT or its underlying data (e.g., DROP TABLE, ALTER
> TABLE, INSERT, UPDATE), which impact the operation on CLT or update
> the CLT data.
> 2. Don't block Indirect/Edge-Case Operations: We should not write
> custom code just to block edge cases that don't directly modify CLT
> data or impact the operations on CLT. For example, if a user decides
> to inherit from a CLT, that constitutes unexpected usage. We already
> document (or can document) that dropping a subscription internally
> drops the associated CLT. If a user inherits from it anyway and their
> child tables are impacted when the subscription is dropped, that is
> expected behavior and their usage issue.

Fair enough. I'm okay with this approach, provided we document it
clearly, perhaps as a CAUTION: users must be aware that DROP
SUBSCRIPTION cascades the drop to the CLT and all its dependent
objects, including any user-created inherited tables, view etc