I had the same problem and thought similar thing.
At Wed, 16 Mar 2016 11:48:10 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in
<16068.1458143...@sss.pgh.pa.us>
> Robert Haas <robertmh...@gmail.com> 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(roleTup))
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("role with OID %u does not exist", roleid)));
+
rform = (Form_pg_authid) GETSTRUCT(roleTup);
- roleid = HeapTupleGetOid(roleTup);
rname = NameStr(rform->rolname);
-
AuthenticatedUserId = roleid;
AuthenticatedUserIsSuperuser = rform->rolsuper;
@@ -524,6 +529,8 @@ InitializeSessionUserId(const char *rolename, Oid roleid)
/* (We assume this is an atomic store so no lock is needed) */
MyProc->roleId = roleid;
+ UnlockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock);
+
/*
* These next checks are not enforced when in standalone mode, so that
* there is a way to recover from sillinesses like "UPDATE pg_authid SET
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index dd37c0c..8c69d6f 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -62,6 +62,8 @@ extern bool HaveVirtualXIDsDelayingChkpt(VirtualTransactionId *vxids, int nvxids
extern PGPROC *BackendPidGetProc(int pid);
extern PGPROC *BackendPidGetProcWithLock(int pid);
+extern bool BackendRoleProcExists(Oid roleoid);
+extern bool BackendRoleProcExistsWithLock(Oid roleoid);
extern int BackendXidGetPid(TransactionId xid);
extern bool IsBackendPid(int pid);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
