On Thu, Oct 29, 2026 at 2:21 PM shveta malik <[email protected]> wrote: > > On Wed, Oct 28, 2026 at 4:34 PM Dilip Kumar <[email protected]> wrote: > > > > On Wed, Oct 28, 2026 at 2:28 PM shveta malik <[email protected]> wrote: > > > > > > On Wed, Oct 28, 2026 at 10:50 AM shveta malik <[email protected]> > > > wrote: > > > > > > > > On Tue, Oct 27, 2026 at 5:03 PM Nisha Moond <[email protected]> > > > > wrote: > > > > > > > > > > Please find the updated patches v85. I also incorporated Shveta's doc > > > > > suggestion[1] into the v85-0004 patch. > > > > > > > > Thanks for the patches. You did not post v85-0004, which you are > > > > referring to in the above comment. > > > > > > > > I reviewed 001 mainly. A few comments: > > > > > > > > > > Reviewed 002 and 003, one minor comment in 002, we have error messages > > > like below: > > > > > > + errdetail("Dropping the conflict log table requires a lock that > > > prepared transactions of this subscription may be holding."), > > > > > > + errdetail("Prepared transactions of the subscription may hold a lock > > > on its conflict log table."), > > > > > > The second message uses the ambiguous "its". It would be clearer to > > > explicitly refer to the 'subscription’s conflict log table', as in the > > > first message. > > > > > > Suggestion: > > > "Prepared transactions of the subscription may hold a lock on the > > > subscription’s conflict log table." > > > > Attached patch set fixes all the comments of 0001 and 0002. It also > > includes Nisha's offlist top up patch for 0001 and 0002, along with > > some offlist suggestions from Amit. Additionally I have removed this > > line "Renamed the conflict log table column remote_origin to > > apply_origin." from the commit message, because this change is not on > > top of the HEAD, it was just changed over previous version of the same > > patch. > > > > Just one trivial thing, in 002's new change in > AlterSubscriptionOwner_internal(), it would be good to change comment > slightly from 'while the prepared transaction waits for the apply > worker' to 'while we wait for the prepared transaction to be resolved > by the apply worker' for better clarity.
That makes sense; I will include this change in the next version, thanks. -- Regards, Dilip Kumar Google
