Hi, I looked at v89 of the patch, I had just one small comment (different from the comments posted already),
In insert_conflict_log_tuple(): we have the following lines + if (remote_commit_ts > 0) + values[attno++] = TimestampTzGetDatum(remote_commit_ts); + else + nulls[attno++] = true; I think "if (remote_commit_ts != 0)" would be better for the following reasons, 1. build_local_conflicts_json_array() just above checks "if (conflicttuple->ts)" 2. The patch also has the line, + TimestampTz commit_ts; /* remote commit ts; 0 if unknown */ we explicitly define 0 as the sentinel when the remote commit timestamp is unknown Thank you, Narayanan On Tue, Nov 3, 2026 at 4:16 PM 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 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.
