On Tue, 9 Jun 2026 at 9:18 AM, shveta malik <[email protected]> wrote:
> On Mon, Jun 8, 2026 at 7:01 PM Dilip Kumar <[email protected]> wrote: > > > > On Mon, Jun 8, 2026 at 11:43 AM shveta malik <[email protected]> > wrote: > > > > > > On Fri, Jun 5, 2026 at 4:22 PM Dilip Kumar <[email protected]> > wrote: > > > > > > > > On Fri, Jun 5, 2026 at 3:06 PM shveta malik <[email protected]> > wrote: > > > > > > > > > > On Fri, Jun 5, 2026 at 11:53 AM Dilip Kumar <[email protected]> > wrote: > > > > > > > > > > > > On Thu, Jun 4, 2026 at 4:05 PM shveta malik < > [email protected]> 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 Make sense.. >
