From d7761b315818243e48cb0cab2829951db5f13f91 Mon Sep 17 00:00:00 2001
From: Jodi-Ann Francis <53411733+francisjodi@users.noreply.github.com>
Date: Thu, 7 Sep 2023 12:12:16 -0400
Subject: [PATCH] Review changes for pg16 update GRANT and REVOKE

---
 src/backend/distributed/commands/role.c       | 12 -----
 .../distributed/deparser/deparse_role_stmts.c | 53 +++++++++++++------
 src/test/regress/sql/pg16.sql                 | 37 +------------
 3 files changed, 38 insertions(+), 64 deletions(-)

diff --git a/src/backend/distributed/commands/role.c b/src/backend/distributed/commands/role.c
index be3a27cbc..4f978cc0a 100644
--- a/src/backend/distributed/commands/role.c
+++ b/src/backend/distributed/commands/role.c
@@ -883,18 +883,6 @@ GenerateGrantRoleStmtsOfRole(Oid roleid)
        		DefElem *set_opt = makeDefElem("set", (Node *) makeBoolean(true), -1);
         	grantRoleStmt->opt = list_make3(opt, inherit_opt, set_opt);
 		}
-
-		if (membership->inherit_option)
-		{
-   		 	DefElem *opt = makeDefElem("inherit", (Node *) makeBoolean(true), -1);
-    		grantRoleStmt->opt = lappend(grantRoleStmt->opt, opt);
-		}
-		
-		if (membership->set_option)
-		{
-    		DefElem *opt = makeDefElem("set", (Node *) makeBoolean(true), -1);
-    		grantRoleStmt->opt = lappend(grantRoleStmt->opt, opt);
-		}
 #else
 		grantRoleStmt->admin_opt = membership->admin_option;
 #endif
diff --git a/src/backend/distributed/deparser/deparse_role_stmts.c b/src/backend/distributed/deparser/deparse_role_stmts.c
index 7379d1a99..419a106cc 100644
--- a/src/backend/distributed/deparser/deparse_role_stmts.c
+++ b/src/backend/distributed/deparser/deparse_role_stmts.c
@@ -409,16 +409,28 @@ AppendRevokeAdminOptionFor(StringInfo buf, GrantRoleStmt *stmt)
 		{
 			switch (opt->defname)
 			{
-				appendStringInfo(buf, "ADMIN OPTION FOR ");
-			}
-			else if (strcmp(opt->defname, "inherit") == 0);
-			{
-				appendStringInfo(buf, "INHERIT TRUE");
-				appendStringInfo(buf, "GRANT x TO y WITH INHERIT TRUE, SET TRUE;");
-			}
-			else if (strcmp(opt->defname, "set") == 0)
-			{
-				appendStringInfo(buf, "SET TRUE"); 
+				case "admin":
+					appendStringInfo(buf, "ADMIN OPTION FOR ");
+					opt_count++;
+					break;
+
+				case "inherit":
+					if (opt_count > 0)
+					{
+						appendStringInfo(buf, ", ");
+					}
+					appendStringInfo(buf, "INHERIT OPTION FOR ");
+					opt_count++;
+					break;
+
+				case "set":
+					if (opt_count > 0)
+					{
+						appendStringInfo(buf, ", ");
+					}
+					appendStringInfo(buf, "SET OPTION FOR ");
+					opt_count++;
+					break;
 			}
 		}
 	}
@@ -438,14 +450,23 @@ AppendGrantWithAdminOption(StringInfo buf, GrantRoleStmt *stmt)
 	{
 #if PG_VERSION_NUM >= PG_VERSION_16
 		DefElem *opt = NULL;
+		 int opt_count = 0;
 		foreach_ptr(opt, stmt->opt)
 		{
-			bool admin_option = false;
-			char *optval = defGetString(opt);
-			if (strcmp(opt->defname, "admin") == 0 &&
-				parse_bool(optval, &admin_option) && admin_option)
-			{
-				appendStringInfo(buf, " WITH ADMIN OPTION");
+			switch (opt->defname)
+        {
+            case "admin":
+                appendStringInfo(buf, " WITH ADMIN OPTION");
+                opt_count++;
+                break;
+
+			case "inherit":
+				if (opt_count > 0)
+				{
+					appendStringInfo(buf, ", ");
+				}
+				appendStringInfo(buf, "INHERIT OPTION ");
+				opt_count++;
 				break;
 
 				
diff --git a/src/test/regress/sql/pg16.sql b/src/test/regress/sql/pg16.sql
index 61cfe17a5..55be0a405 100644
--- a/src/test/regress/sql/pg16.sql
+++ b/src/test/regress/sql/pg16.sql
@@ -913,39 +913,4 @@ GRANT SET SESSION AUTHORIZATION TO role_name;
 
 SELECT * FROM table_name WHERE column_name = 'value';
 
-SELECT COUNT(*) FROM table_name WHERE column_name = 'value';
-
---
--- PG16 allows GRANT WITH ADMIN | INHERIT | SET
---
--- GRANT privileges to a role or roles  
-\c - - - :master_port
-CREATE ROLE create_role;
-CREATE ROLE create_role_2;
-CREATE ROLE create_role_3;
-CREATE ROLE create_role_4;
-CREATE USER create_user;
-CREATE USER create_user_2;
-CREATE GROUP create_group;
-CREATE GROUP create_group_2;
-
---test grant role
-GRANT create_group TO create_role;
-GRANT create_group TO create_role_2 WITH ADMIN OPTION;
-GRANT create_group TO create_role_3 WITH INHERIT;
-GRANT create_group TO create_role_4 WITH SET; 
-
--- ADMIN role can perfom administrative tasks 
--- role can now access the data and permissions of the table (owner of table)
--- role can change current user to any other user/role that has access 
-GRANT ADMIN TO joe;
-GRANT INHERIT ON ROLE joe TO james;
-
-GRANT SELECT ON companies TO joe WITH GRANT OPTION;
-GRANT SET (SELECT) ON companies TO james;
-
-
-\set VERBOSITY terse
-SET client_min_messages TO ERROR;
-DROP SCHEMA pg16 CASCADE;
-
+SELECT COUNT(*) FROM table_name WHERE column_name = 'value';
\ No newline at end of file