On Tue, Nov 3, 2026 at 3:17 PM shveta malik <[email protected]> wrote: > > On Tue, Nov 3, 2026 at 1:47 PM Masahiko Sawada <[email protected]> wrote: > > > > Hi, > > > > On Mon, Nov 2, 2026 at 3:17 AM Amit Kapila <[email protected]> wrote: > > > > > > On Sun, Nov 1, 2026 at 8:17 PM shihao zhong <[email protected]> wrote: > > > > > > > > Thanks for addressing all of them. > > > > > > > > Lgtm > > > > > > > > > > Thanks, here is a combined patch after fixing cosmetic issues > > > including combining one test case with an existing test. I am planning > > > to commit this tomorrow unless you or others have more comments on it. > > > Sawada-san, the issue related to autovacuum cancellation during > > > truncation raised by you was also addressed, please do let me know if > > > you need more time to review? > > > > I've reviewed the v88 patch and have some comments and questions: > > > > In lock_conflictlog_relation(), LookupGXactBySubid() doesn't check > > whether the prepared transaction actually holds a lock on the CLT, so > > there are false positives. For example, when autovacuum is truncating > > the CLT and the apply worker attempts to acquire a lock on it, the > > apply worker raises an error even if the prepared transaction created > > by the subscription doesn't hold any lock on the CLT. Since the check > > is not precise, I think the subscription could error out easily. For > > example, with disable_on_error = true, users would need to re-enable > > the subscription frequently, which doesn't seem good. > > I agree with the side-effect of these false-positive cases. But adding > lock-acquisition checks could complicate the code further. >
Yes, I also thought about it previously and felt the same. To achieve this we need to scan the lock table for holders on the relation and match them to the subscription's prepared transactions (their dummy PGPROCs). See attached top-up atop v88 generated by AI. I can review/test it more if we decide to go with this strategy. I still feel we shouldn't add this because such scenarios will be rare in practice. It won't be difficult to address this if we really see such field reports, so I would like to leave this code as is but if you still think otherwise then we can take care with finding whether prepared xact has locked CLT. I think we > can simply drop the fast-error block in lock_conflictlog_relation(): > > if (LookupGXactBySubid(MySubscription->oid)) > ereport(ERROR, ... "could not obtain lock on conflict log table ..." ...); > > and let the worker fall through to the bounded wait (lock_timeout = > deadlock_timeout + 5s) in all cases. Wouldn't that resolve the > problem? > > IMO, this should eliminate the false positive where a blocking > autovacuum is mistaken for a prepared transaction. A regular > autovacuum should yield or be cancelled within that window, allowing > the worker to acquire the lock and continue. A genuine prepared > transaction, on the other hand, will continue holding the lock, > causing only that case to reach the timeout and error. Thoughts? > Yeah, this sounds better than the above idea shared by me. But this also won't be precise in telling the reason in case it is really blocked by prepared xact, OTOH, maybe we don't need to care about the same. Let us see what others think. > > --- > > Besides prepared transactions actually created by the apply worker, > > IsTwoPhaseTransactionGidForSubid() checks only the GID format, so any > > user can create a prepared transaction named pg_gid_<subid>_<xid>, > > even an empty one. This makes the apply worker of a two_phase > > subscription error out more easily, and it can also block DROP > > SUBSCRIPTION and ALTER SUBSCRIPTION ... SET (conflict_log_destination > > = 'log'). > > Right but do we really need to worry about such cases? For example, one can say the same about replication slots, say a user has created a slot with name pg_xxx as generated internally by Create Subscription then even Create Subscription can fail. I didn't notice such reports, so I'm not sure if we should try to take care of this. > > --- > > ALTER SUBSCRIPTION ... OWNER TO is allowed even when the subscription > > has prepared transactions. It waits for the lock held by the prepared > > transaction, and the next conflict makes the apply worker error out. > > Should we prohibit it as we do for DROP SUBSCRIPTION and changing > > conflict_log_destination to 'log'? > > DROP SUBSCRIPTION and ALTER SUBSCRIPTION behaves differently because they will prevent new workers from starting, so there won't be a chance that a COMMIT PREPARED is applied and the dead lock is released. But for the ALTER SUBSCRIPTION ... OWNER case, the same issue won't happen, so it's more similar to the TRUNCATE command, and we didn't added checks but we can add one if you think that is really worth it. Again here there is a chance of false positive due to unrelated Prepared Transactions unless we follow something like attached nocfbot-v88-check_clt_locked_by_prepared_xact. > > --- > > Is there any way to carry over CLT data across major versions? > > Yes, there is a patch for this which is under discussion earlier, see [1] but we thought we can deal with insertion as a pre-step and then review that more care fully. > > --- > > The xid in local_conflicts is a 32-bit xid. I think it's better to > > record a FullTransactionId instead. Otherwise, after xid wraparound, > > two different local transactions can be recorded with the same xid, > > which would easily confuse monitoring queries. We can't do the same > > for remote_xid since the protocol sends only a 32-bit xid, but it > > might be worth documenting. > > Agreed. [1]: https://www.postgresql.org/message-id/CAD21AoBNTnF6Xzo-UqGxALNqyy85EFsPWDRuXGk0aibD2ugkgQ%40mail.gmail.com -- With Regards, Amit Kapila.
nocfbot-v88-check_clt_locked_by_prepared_xact.patch
Description: Binary data
