On Tue, Nov 3, 2026 at 2:31 AM Amit Kapila <[email protected]> wrote: > > 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'm not sure it's that rare. Tuples copied by the initial table sync always cause a conflict (update_origin_differs or delete_origin_differs) when the apply worker modifies them for the first time, if track_commit_timestamp is enabled. Also, in systems using two-phase commit, it's common that the subscription has prepared transactions. But I might be worrying too much. TBH I'm still not sure why we really need the special behavior of erroring out only for the CLT. I agree that it's an internal behavior and users might not be aware of it, but I think users who don't know it would be equally confused when the apply worker errors out because of it. > > > > --- > > > 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. I think it's a bit different from the replication slot case. Creating a replication slot requires the REPLICATION privilege, which is a high privilege. On the other hand, any user can create a prepared transaction. > > > > --- > > > 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. Agreed. > > > > --- > > > 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. Nice. Thank you for letting me know. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
