Re: [HACKERS] Show dropped users' backends in pg_stat_activity
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> I concur. Let's put the left join(s) into those views and call it >> good. > I'd suggest we also add some notes to the documentation that the correct > approach to dropping users is to disallow access first, then kill any > existing backends, and then drop the user. That, plus the left joins, > seems like it's good enough. I've pushed Oskari's original patch; a look around in system_views.sql shows that it's justified on the basis of consistency with other views even aside from the functional problem he ran into. The documentation question seems like material for a separate patch. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
On Thu, Apr 7, 2016 at 11:22:08PM +0300, Oskari Saarenmaa wrote: > 24.03.2016, 18:03, Tom Lane kirjoitti: > >Robert Haas writes: > >>I am not really in favor of half-fixing this. If we can't > >>conveniently wait until a dropped role is completely out of the > >>system, then I don't see a lot of point in trying to do it in the > >>limited cases where we can. If LEFT JOIN is the way to go, then, > >>blech, but, so be it. > > > >I concur. Let's put the left join(s) into those views and call it > >good. > > > >BTW, I think we would need the left joins even if we had interlocking > >in DROP, just to protect ourselves against race conditions. Remember > >that what pg_stat_activity shows is a snapshot, which might be more or > >less out of date compared to the catalog contents. > > Added my patch to the 2016-09 commitfest > (https://commitfest.postgresql.org/10/601/) as a bug fix as I thought not > showing all backends in pg_stat_activity is a bug. Any chance to get it in > 9.6? Do we need a comment in the query explaining why a left join is needed, e.g. "Use LEFT JOIN in case the role has been dropped"? That wouldn't be obvious to me. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
Oskari Saarenmaa writes: > 24.03.2016, 18:03, Tom Lane kirjoitti: >> I concur. Let's put the left join(s) into those views and call it >> good. > Added my patch to the 2016-09 commitfest > (https://commitfest.postgresql.org/10/601/) as a bug fix as I thought > not showing all backends in pg_stat_activity is a bug. Any chance to > get it in 9.6? I do not think this is a sufficiently high-priority bug to justify putting it into 9.6 at this point. It's been like this for years and there's a noticeable lack of any field complaints. Fixing it in the next release cycle seems fine to me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
24.03.2016, 18:03, Tom Lane kirjoitti: Robert Haas writes: I am not really in favor of half-fixing this. If we can't conveniently wait until a dropped role is completely out of the system, then I don't see a lot of point in trying to do it in the limited cases where we can. If LEFT JOIN is the way to go, then, blech, but, so be it. I concur. Let's put the left join(s) into those views and call it good. BTW, I think we would need the left joins even if we had interlocking in DROP, just to protect ourselves against race conditions. Remember that what pg_stat_activity shows is a snapshot, which might be more or less out of date compared to the catalog contents. Added my patch to the 2016-09 commitfest (https://commitfest.postgresql.org/10/601/) as a bug fix as I thought not showing all backends in pg_stat_activity is a bug. Any chance to get it in 9.6? -- Oskari Saarenmaa Aiven: managed cloud databases https://aiven.io -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haas writes: > > I am not really in favor of half-fixing this. If we can't > > conveniently wait until a dropped role is completely out of the > > system, then I don't see a lot of point in trying to do it in the > > limited cases where we can. If LEFT JOIN is the way to go, then, > > blech, but, so be it. > > I concur. Let's put the left join(s) into those views and call it > good. I'd suggest we also add some notes to the documentation that the correct approach to dropping users is to disallow access first, then kill any existing backends, and then drop the user. That, plus the left joins, seems like it's good enough. > BTW, I think we would need the left joins even if we had interlocking > in DROP, just to protect ourselves against race conditions. Remember > that what pg_stat_activity shows is a snapshot, which might be more or > less out of date compared to the catalog contents. True, though that would likely be a much smaller set of cases that might also be short lived. Might be good to also note in the docs how to kill off sessions which are regular users but which no longer have a username, for folks who end up in this situation that they managed to drop a role which still had connections to the system. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
Robert Haas writes: > I am not really in favor of half-fixing this. If we can't > conveniently wait until a dropped role is completely out of the > system, then I don't see a lot of point in trying to do it in the > limited cases where we can. If LEFT JOIN is the way to go, then, > blech, but, so be it. I concur. Let's put the left join(s) into those views and call it good. BTW, I think we would need the left joins even if we had interlocking in DROP, just to protect ourselves against race conditions. Remember that what pg_stat_activity shows is a snapshot, which might be more or less out of date compared to the catalog contents. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
On Tue, Mar 22, 2016 at 11:35 PM, Kyotaro HORIGUCHI wrote: > Even if blocking DROPs is not perfect for all cases, > unconditionally allowing to DROP a role still doesn't seem proper > behavior, especially for replication roles. And session logins > seem to me to have enough reason to be treated differently than > disguising as another role using SET ROLE or sec-definer. > > The attached patch blocks DROP ROLE for roles that own active > sessions, and on the other hand prevents a session from being > activated if the login role is concurrently dropped. > > Oskari's LEFT-Join patch is still desirable. > > Is this still pointless? I am not really in favor of half-fixing this. If we can't conveniently wait until a dropped role is completely out of the system, then I don't see a lot of point in trying to do it in the limited cases where we can. If LEFT JOIN is the way to go, then, blech, but, so be it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
Hi, At Tue, 22 Mar 2016 22:47:16 -0500, Jim Nasby wrote in <56f211c4.6010...@bluetreble.com> > On 3/22/16 10:35 PM, Kyotaro HORIGUCHI wrote: > >> Even if we maintained some interlock for a backend's login role > >> identity, > >> >I hardly think it would be practical to e.g. lock during transient SET > >> >ROLE or security-definer-function-call operations. So it's not like > >> >we > >> >can let the permissions system assume that a role OID being inquired > >> >about > >> >always matches a live entry in pg_authid. > > Even if blocking DROPs is not perfect for all cases, > > unconditionally allowing to DROP a role still doesn't seem proper > > behavior, especially for replication roles. And session logins > > seem to me to have enough reason to be treated differently than > > disguising as another role using SET ROLE or sec-definer. > > There's probably a way this could be handled, since DROP ROLE is > presumably a very uncommon operation. Perhaps something as simple as > keeping a single OID in shared memory for the role about to be > dropped. That would serialize role drops, but I doubt that matters. The OID in shared memory has the same role with a tuple with the OID in pg_authid in this patch. So it seems need a lock or a retry mechanism, or we see a message something like this:p | DROP ROLE: Another role is concurrently being dropped. > > The attached patch blocks DROP ROLE for roles that own active > > sessions, and on the other hand prevents a session from being > > activated if the login role is concurrently dropped. > > I think this is fine for now, but... what happens if you drop a role > that's in use on a streaming replica? Does replay stall or do we just > ignore it? It behaves as the same to the ordinary backends. DROP ROLE fails for any active walsender's session(?) role, or a new walsender rejects login attempts by the role under being dropped. > There should probably be some doc changes to go with the patch too, > no? Yes, this is a PoC. I'll provide documentation if this is acceptable, and necessary. "20.4 Dropping Roles" would be appropriate? http://www.postgresql.org/docs/9.5/static/role-removal.html Treating a session as an object dependent on the role could be cleaner but may be too complex and fragile.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
On 3/22/16 10:35 PM, Kyotaro HORIGUCHI wrote: Even if we maintained some interlock for a backend's login role identity, >I hardly think it would be practical to e.g. lock during transient SET >ROLE or security-definer-function-call operations. So it's not like we >can let the permissions system assume that a role OID being inquired about >always matches a live entry in pg_authid. Even if blocking DROPs is not perfect for all cases, unconditionally allowing to DROP a role still doesn't seem proper behavior, especially for replication roles. And session logins seem to me to have enough reason to be treated differently than disguising as another role using SET ROLE or sec-definer. There's probably a way this could be handled, since DROP ROLE is presumably a very uncommon operation. Perhaps something as simple as keeping a single OID in shared memory for the role about to be dropped. That would serialize role drops, but I doubt that matters. The attached patch blocks DROP ROLE for roles that own active sessions, and on the other hand prevents a session from being activated if the login role is concurrently dropped. I think this is fine for now, but... what happens if you drop a role that's in use on a streaming replica? Does replay stall or do we just ignore it? There should probably be some doc changes to go with the patch too, no? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
I had the same problem and thought similar thing. At Wed, 16 Mar 2016 11:48:10 -0400, Tom Lane wrote in <16068.1458143...@sss.pgh.pa.us> > Robert Haas writes: > > Gee, I would have expected the DROP to be blocked until the user > > disconnected, like we do for DROP DATABASE. FWTW, I agree with Robert. > Making that race-condition-free would require some notion of a lock on > roles, I think. Seems pretty messy compared to the amount of actual > value obtained. There are good reasons why you can't have a backend > running in a nonexistent database; but a backend with a nonexistent > user OID is not really going to be a problem for anything except > monitoring queries that fail to use left joins where appropriate. > > Even if we maintained some interlock for a backend's login role identity, > I hardly think it would be practical to e.g. lock during transient SET > ROLE or security-definer-function-call operations. So it's not like we > can let the permissions system assume that a role OID being inquired about > always matches a live entry in pg_authid. Even if blocking DROPs is not perfect for all cases, unconditionally allowing to DROP a role still doesn't seem proper behavior, especially for replication roles. And session logins seem to me to have enough reason to be treated differently than disguising as another role using SET ROLE or sec-definer. The attached patch blocks DROP ROLE for roles that own active sessions, and on the other hand prevents a session from being activated if the login role is concurrently dropped. Oskari's LEFT-Join patch is still desirable. Is this still pointless? regards, diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 4baeaa2..52ac271 100644 --- a/src/backend/commands/user.c +++ b/src/backend/commands/user.c @@ -31,6 +31,7 @@ #include "libpq/md5.h" #include "miscadmin.h" #include "storage/lmgr.h" +#include "storage/procarray.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/fmgroids.h" @@ -1026,6 +1027,12 @@ DropRole(DropRoleStmt *stmt) errdetail_internal("%s", detail), errdetail_log("%s", detail_log))); + /* If this role is currently connecting, refuse to drop it. */ + if (BackendRoleProcExists(roleid)) + ereport(ERROR, + (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST), + errmsg("role \"%s\" is currently logged in", role))); + /* * Remove the role from the pg_authid table */ diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 740beb6..ad208d7 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -2358,6 +2358,54 @@ BackendPidGetProcWithLock(int pid) } /* + * BackendRoleProcExists -- check if a backend with given its role exists + * + * Returns true if found. + */ +bool +BackendRoleProcExists(Oid roleoid) +{ + bool result; + + if (roleoid == 0) /* No match with role id 0 */ + return false; + + LWLockAcquire(ProcArrayLock, LW_SHARED); + + result = BackendRoleProcExistsWithLock(roleoid); + + LWLockRelease(ProcArrayLock); + + return result; +} + +/* + * BackendRoleProcExistsWithLock -- check if a backend with given its role + * exists + * + * Same as above, except caller must be holding ProcArrayLock. + */ +bool +BackendRoleProcExistsWithLock(Oid roleoid) +{ + ProcArrayStruct *arrayP = procArray; + int index; + + if (roleoid == 0) /* No match with role id 0 */ + return false; + + for (index = 0; index < arrayP->numProcs; index++) + { + PGPROC *proc = &allProcs[arrayP->pgprocnos[index]]; + + if (proc->roleId == roleoid) + return true; + } + + return false; +} + +/* * BackendXidGetPid -- get a backend's pid given its XID * * Returns 0 if not found or it's a prepared transaction. Note that diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index d13355b..f115c43 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -41,6 +41,7 @@ #include "storage/fd.h" #include "storage/ipc.h" #include "storage/latch.h" +#include "storage/lmgr.h" #include "storage/pg_shmem.h" #include "storage/proc.h" #include "storage/procarray.h" @@ -500,20 +501,24 @@ InitializeSessionUserId(const char *rolename, Oid roleid) ereport(FATAL, (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), errmsg("role \"%s\" does not exist", rolename))); - } - else - { - roleTup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); - if (!HeapTupleIsValid(roleTup)) - ereport(FATAL, - (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), - errmsg("role with OID %u does not exist", roleid))); + roleid = HeapTupleGetOid(roleTup); + ReleaseSysCache(roleTup); } + /* + * Inhibiting this session from being activated for concurrently dropped + * roles + */ + LockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock); + + roleTup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid)); + if (!HeapTupleIsValid
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
Robert Haas writes: > Gee, I would have expected the DROP to be blocked until the user > disconnected, like we do for DROP DATABASE. Making that race-condition-free would require some notion of a lock on roles, I think. Seems pretty messy compared to the amount of actual value obtained. There are good reasons why you can't have a backend running in a nonexistent database; but a backend with a nonexistent user OID is not really going to be a problem for anything except monitoring queries that fail to use left joins where appropriate. Even if we maintained some interlock for a backend's login role identity, I hardly think it would be practical to e.g. lock during transient SET ROLE or security-definer-function-call operations. So it's not like we can let the permissions system assume that a role OID being inquired about always matches a live entry in pg_authid. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
On Tue, Mar 15, 2016 at 5:21 PM, Oskari Saarenmaa wrote: > I was looking into some issues we recently had when dropping db users and > was surprised to see that dropped users' sessions and transactions continue > to work after the role is dropped. > > Since dropping a role requires dropping all grants it has (using DROP OWNED > BY ...) the dropped role can't start new transactions that do a whole lot > unless there are objects with access granted to PUBLIC, but any running > transactions remain running and can write to the database. They can also > hold locks which interfere with other backends without showing up in most > activity or lock monitoring tools as they won't appear in pg_stat_activity. > > IMO any open sessions for a dropped user should be automatically terminated > when the role is dropped, but that would probably be a bigger change so > attached a proposed patch for using left joins in pg_stat_activity and > pg_stat_replication to show activity by dropped roles. Gee, I would have expected the DROP to be blocked until the user disconnected, like we do for DROP DATABASE. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] Show dropped users' backends in pg_stat_activity
16.03.2016, 17:48, Tom Lane kirjoitti: Robert Haas writes: Gee, I would have expected the DROP to be blocked until the user disconnected, like we do for DROP DATABASE. Making that race-condition-free would require some notion of a lock on roles, I think. Seems pretty messy compared to the amount of actual value obtained. There are good reasons why you can't have a backend running in a nonexistent database; but a backend with a nonexistent user OID is not really going to be a problem for anything except monitoring queries that fail to use left joins where appropriate. I don't think most people expect dropped users to be able to keep using the database. If it's not feasible to block DROP ROLE until the user has disconnected or to kill dropped users' sessions immediately after they're dropped we should at least show their sessions in pg_stat_activity and add a note about it in DROP ROLE docs. / Oskari -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers