On 2020-Jan-14, Alvaro Herrera wrote:
> On 2020-Jan-13, Tom Lane wrote:
>
> > That seems fundamentally wrong. By the time we've queued an object for
> > deletion in dependency.c, we have a lock on it, and we've verified that
> > the object is still there (cf. systable_recheck_tuple calls).
> > If shdepDropOwned is doing it differently, I'd say shdepDropOwned is
> > doing it wrong.
>
> Hmm, it seems to be doing it differently. Maybe it should be acquiring
> locks on all objects in that nested loop and verified them for
> existence, so that when it calls performMultipleDeletions the objects
> are already locked, as you say.
Yeah, this solves the reported bug.
This is not a 100% solution: there's the cases when the user is removed
from an ACL and from a policy, and those deletions are done directly
instead of accumulating to the end for a mass deletion.
I had to export AcquireDeletionLock (previously a static in
dependency.c). I wonder if I should export ReleaseDeletionLock too, for
symmetry.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c4a4df25b8..2ec3e39694 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -199,7 +199,6 @@ static void reportDependentObjects(const ObjectAddresses *targetObjects,
static void deleteOneObject(const ObjectAddress *object,
Relation *depRel, int32 flags);
static void doDeletion(const ObjectAddress *object, int flags);
-static void AcquireDeletionLock(const ObjectAddress *object, int flags);
static void ReleaseDeletionLock(const ObjectAddress *object);
static bool find_expr_references_walker(Node *node,
find_expr_references_context *context);
@@ -1530,7 +1529,7 @@ doDeletion(const ObjectAddress *object, int flags)
* else. Note that dependency.c is not concerned with deleting any kind of
* shared-across-databases object, so we have no need for LockSharedObject.
*/
-static void
+void
AcquireDeletionLock(const ObjectAddress *object, int flags)
{
if (object->classId == RelationRelationId)
diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 2ef792dbd7..20d7ee7d87 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1319,12 +1319,16 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
elog(ERROR, "unexpected dependency type");
break;
case SHARED_DEPENDENCY_ACL:
+ /* XXX need to lock+accumulate everything and drop later? */
RemoveRoleFromObjectACL(roleid,
sdepForm->classid,
sdepForm->objid);
break;
case SHARED_DEPENDENCY_POLICY:
- /* If unable to remove role from policy, remove policy. */
+ /*
+ * Try to remove role from policy; if unable to, remove
+ * policy.
+ */
if (!RemoveRoleFromObjectPolicy(roleid,
sdepForm->classid,
sdepForm->objid))
@@ -1332,6 +1336,15 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
obj.classId = sdepForm->classid;
obj.objectId = sdepForm->objid;
obj.objectSubId = sdepForm->objsubid;
+ /*
+ * Acquire lock on object, then verify this dependency
+ * is still relevant. If not, the object might have
+ * been dropped or the policy modified. Ignore the
+ * object in that case.
+ */
+ AcquireDeletionLock(&obj, 0);
+ if (!systable_recheck_tuple(scan, tuple))
+ break;
add_exact_object_address(&obj, deleteobjs);
}
break;
@@ -1342,6 +1355,10 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
obj.classId = sdepForm->classid;
obj.objectId = sdepForm->objid;
obj.objectSubId = sdepForm->objsubid;
+ /* as above */
+ AcquireDeletionLock(&obj, 0);
+ if (!systable_recheck_tuple(scan, tuple))
+ break;
add_exact_object_address(&obj, deleteobjs);
}
break;
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 0cd6fcf027..fa9252b07c 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -142,6 +142,8 @@ typedef enum ObjectClass
/* in dependency.c */
+extern void AcquireDeletionLock(const ObjectAddress *object, int flags);
+
extern void performDeletion(const ObjectAddress *object,
DropBehavior behavior, int flags);
