On Thu, Dec 9, 2021 at 10:52 PM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > > > On Dec 9, 2021, at 7:41 AM, Robert Haas <robertmh...@gmail.com> wrote: > > > > On Tue, Nov 30, 2021 at 6:55 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > >>> This patch does detect ownership changes more quickly (at the > >>> transaction boundary) than the current code (only when it reloads for > >>> some other reason). Transaction boundary seems like a reasonable time > >>> to detect the change to me. > >>> > >>> Detecting faster might be nice, but I don't have a strong opinion about > >>> it and I don't see why it necessarily needs to happen before this patch > >>> goes in. > >> > >> I think it would be better to do it before we allow subscription > >> owners to be non-superusers. > > > > I think it would be better not to ever do it at any time. > > > > It seems like a really bad idea to me to change the run-as user in the > > middle of a transaction. > > I agree. We allow SET ROLE inside transactions, but faking one on the > subscriber seems odd. No such role change was performed on the publisher > side, nor is there a principled reason for assuming the old run-as role has > membership in the new run-as role, so we'd be pretending to do something that > might otherwise be impossible. > > There was some discussion off-list about having the apply worker take out a > lock on its subscription, thereby blocking ownership changes mid-transaction. > I coded that and it seems to work fine, but I have a hard time seeing how > the lock traffic would be worth expending. Between (a) changing roles > mid-transaction, and (b) locking the subscription for each transaction, I'd > prefer to do neither, but (b) seems far better than (a). Thoughts? >
Yeah, to me also (b) sounds better than (a). However, a few points that we might want to consider in that regard are as follows: 1. locking the subscription for each transaction will add new blocking areas considering we acquire AccessExclusiveLock to change any property of subscription. But as Alter Subscription won't be that frequent operation it might be acceptable. 2. It might lead to adding some cost to small transactions but not sure if that will be noticeable. 3. Tomorrow, if we want to make the apply-process parallel (IIRC, we do have the patch for that somewhere in archives) especially for large in-progress transactions then this locking will have additional blocking w.r.t Altering Subscription. But again, this also might be acceptable. -- With Regards, Amit Kapila.