Re: Non-superuser subscription owners
Amit Kapila <amit.kapila16@gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
To: Mark Dilger <mark.dilger@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Jeff Davis <pgsql@j-davis.com>, Andrew Dunstan <andrew@dunslane.net>,
PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-12-10T04:15:08Z
Lists: pgsql-hackers
On Thu, Dec 9, 2021 at 10:52 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > > On Dec 9, 2021, at 7:41 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > > > On Tue, Nov 30, 2021 at 6:55 AM Amit Kapila <amit.kapila16@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.
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix possible crash in tablesync worker.
- b5c517379a40 16.0 landed
-
Display 'password_required' option for \dRs+ command.
- 19e65dff38bd 16.0 landed
-
Restart the apply worker if the 'password_required' option is changed.
- c1cc4e688b60 16.0 landed
-
Fix possible logical replication crash.
- e7e7da2f8d57 16.0 landed
-
Add new predefined role pg_create_subscription.
- c3afe8cf5a1e 16.0 landed
-
Expand AclMode to 64 bits
- 7b378237aa80 16.0 cited
-
More cleanup of a2ab9c06ea.
- 96a6f11c0625 15.0 landed
-
Respect permissions within logical replication.
- a2ab9c06ea15 15.0 landed
-
Improve table locking behavior in the face of current DDL.
- 2ad36c4e44c8 9.2.0 cited