From b7c426251a820824a4bdf8612dcdcd54fb9adc44 Mon Sep 17 00:00:00 2001
From: xiong-gang <gxiong@pivotal.io>
Date: Tue, 3 Nov 2020 10:30:01 +0800
Subject: [PATCH] Fix resgroup unusable if its dropping failed

In function DropResourceGroup(), group->lockedForDrop is set
to true by calling ResGroupCheckForDrop, however, it can only
be set to false inside dropResgroupCallback. This callback is
registered at the ending of function DropResourceGroup. If an
error occured between them, group->lockedForDrop would be true
forever.

Fix it by putting the register process ahead of the lock call.
To prevent Assert(group->nRunning* > 0) if ResGroupCheckForDrop
throws an error, return directly if group->lockedForDrop did
not change.

See:

```
gpconfig -c gp_resource_manager -v group
gpstop -r -a

psql
                CPU_RATE_LIMIT=20,
                MEMORY_LIMIT=20,
                CONCURRENCY=50,
                MEMORY_SHARED_QUOTA=80,
                MEMORY_SPILL_RATIO=20,
                MEMORY_AUDITOR=vmtracker
        );

psql -U user_test
> \d -- hang
```

Co-authored-by: dh-cloud <60729713+dh-cloud@users.noreply.github.com>
---
 src/backend/commands/resgroupcmds.c   | 17 ++++++++---------
 src/backend/utils/resgroup/resgroup.c |  4 ++++
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/backend/commands/resgroupcmds.c b/src/backend/commands/resgroupcmds.c
index e075b055aa..b5541d1e7a 100644
--- a/src/backend/commands/resgroupcmds.c
+++ b/src/backend/commands/resgroupcmds.c
@@ -312,7 +312,15 @@ DropResourceGroup(DropResourceGroupStmt *stmt)
 
 	/* check before dispatch to segment */
 	if (IsResGroupActivated())
+	{
+		/* Argument of callback function should be allocated in heap region */
+		callbackCtx = (ResourceGroupCallbackContext *)
+			MemoryContextAlloc(TopMemoryContext, sizeof(*callbackCtx));
+		callbackCtx->groupid = groupid;
+		RegisterXactCallbackOnce(dropResgroupCallback, callbackCtx);
+
 		ResGroupCheckForDrop(groupid, stmt->name);
+	}
 
 	/*
 	 * Check to see if any roles are in this resource group.
@@ -346,15 +354,6 @@ DropResourceGroup(DropResourceGroupStmt *stmt)
 									NIL,
 									NULL);
 	}
-
-	if (IsResGroupActivated())
-	{
-		/* Argument of callback function should be allocated in heap region */
-		callbackCtx = (ResourceGroupCallbackContext *)
-			MemoryContextAlloc(TopMemoryContext, sizeof(*callbackCtx));
-		callbackCtx->groupid = groupid;
-		RegisterXactCallbackOnce(dropResgroupCallback, callbackCtx);
-	}
 }
 
 /*
diff --git a/src/backend/utils/resgroup/resgroup.c b/src/backend/utils/resgroup/resgroup.c
index b605b761af..05a45dde46 100644
--- a/src/backend/utils/resgroup/resgroup.c
+++ b/src/backend/utils/resgroup/resgroup.c
@@ -3318,6 +3318,8 @@ slotGetId(const ResGroupSlotData *slot)
 static void
 lockResGroupForDrop(ResGroupData *group)
 {
+	if (group->lockedForDrop)
+		return;
 	Assert(LWLockHeldExclusiveByMe(ResGroupLock));
 	Assert(Gp_role == GP_ROLE_DISPATCH);
 	Assert(group->nRunning == 0);
@@ -3328,6 +3330,8 @@ lockResGroupForDrop(ResGroupData *group)
 static void
 unlockResGroupForDrop(ResGroupData *group)
 {
+	if (!group->lockedForDrop)
+		return;
 	Assert(LWLockHeldExclusiveByMe(ResGroupLock));
 	Assert(Gp_role == GP_ROLE_DISPATCH);
 	Assert(group->nRunning == 0);
-- 
GitLab