From 8de0b4a908a2bfe9e679ef4c7745a2c4b90319a5 Mon Sep 17 00:00:00 2001
From: Naisila Puka <37271756+naisila@users.noreply.github.com>
Date: Mon, 11 Sep 2023 15:58:24 +0300
Subject: [PATCH] Fix WITH ADMIN FALSE propagation (#7191)

---
 .../distributed/deparser/deparse_role_stmts.c | 49 ++++---------------
 src/test/regress/expected/pg16.out            | 38 ++++++++++++++
 src/test/regress/sql/pg16.sql                 | 26 ++++++++++
 3 files changed, 73 insertions(+), 40 deletions(-)

diff --git a/src/backend/distributed/deparser/deparse_role_stmts.c b/src/backend/distributed/deparser/deparse_role_stmts.c
index 7dda3e562..a6f52faab 100644
--- a/src/backend/distributed/deparser/deparse_role_stmts.c
+++ b/src/backend/distributed/deparser/deparse_role_stmts.c
@@ -15,6 +15,7 @@
 
 #include "pg_version_compat.h"
 
+#include "commands/defrem.h"
 #include "distributed/citus_ruleutils.h"
 #include "distributed/deparser.h"
 #include "distributed/listutils.h"
@@ -396,47 +397,15 @@ AppendGrantWithAdminOption(StringInfo buf, GrantRoleStmt *stmt)
 		 int opt_count = 0;
 		foreach_ptr(opt, stmt->opt)
 		{
-			switch (opt->defname)
-        {
-            case "admin":
-                appendStringInfo(buf, " WITH ADMIN OPTION");
-                opt_count++;
-                break;
-
-			case "inherit":
-				  if (opt->arg && IsA(opt->arg, A_Const) && !((A_Const *) opt->arg)->val.val.ival)
-				{
-					appendStringInfo(buf, " INHERIT FALSE");
-                }
-                else
-                {
-                    if (opt_count > 0)
-                    {
-                        appendStringInfo(buf, ", ");
-                    }
-                    appendStringInfo(buf, " INHERIT OPTION");
-                    opt_count++;
-                }
-                break;
-				
-        	case "set":
-  				if (opt->arg && IsA(opt->arg, A_Const) && !(( *) opt->arg)->val.val.ival)
-                {
-                    appendStringInfo(buf, " SET FALSE");
-                }
-                else
-                {
-                    if (opt_count > 0)
-                    {
-                        appendStringInfo(buf, ", ");
-                    }
-                    appendStringInfo(buf, " SET OPTION");
-                    opt_count++;
-                }
-                break;
-        }
+			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");
+				break;
+			}
 		}
-	}
 #else
 		if (stmt->admin_opt)
 		{
diff --git a/src/test/regress/expected/pg16.out b/src/test/regress/expected/pg16.out
index 2ed508c90..39fd5a29b 100644
--- a/src/test/regress/expected/pg16.out
+++ b/src/test/regress/expected/pg16.out
@@ -971,6 +971,44 @@ LEFT JOIN ref_table ON TRUE;
             1.19
 (1 row)
 
+--
+-- PG16 added WITH ADMIN FALSE option to GRANT ROLE
+-- WITH ADMIN FALSE is the default, make sure we propagate correctly in Citus
+-- Relevant PG commit: https://github.com/postgres/postgres/commit/e3ce2de
+--
+CREATE ROLE role1;
+CREATE ROLE role2;
+SET citus.log_remote_commands TO on;
+SET citus.grep_remote_commands = '%GRANT%';
+-- default admin option is false
+GRANT role1 TO role2;
+NOTICE:  issuing GRANT role1 TO  role2;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+NOTICE:  issuing GRANT role1 TO  role2;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+REVOKE role1 FROM role2;
+-- should behave same as default
+GRANT role1 TO role2 WITH ADMIN FALSE;
+NOTICE:  issuing GRANT role1 TO  role2;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+NOTICE:  issuing GRANT role1 TO  role2;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+REVOKE role1 FROM role2;
+-- with admin option and with admin true are the same
+GRANT role1 TO role2 WITH ADMIN OPTION;
+NOTICE:  issuing GRANT role1 TO  role2 WITH ADMIN OPTION;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+NOTICE:  issuing GRANT role1 TO  role2 WITH ADMIN OPTION;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+REVOKE role1 FROM role2;
+GRANT role1 TO role2 WITH ADMIN TRUE;
+NOTICE:  issuing GRANT role1 TO  role2 WITH ADMIN OPTION;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+NOTICE:  issuing GRANT role1 TO  role2 WITH ADMIN OPTION;
+DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+REVOKE role1 FROM role2;
+RESET citus.log_remote_commands;
+RESET citus.grep_remote_commands;
 \set VERBOSITY terse
 SET client_min_messages TO ERROR;
 DROP EXTENSION postgres_fdw CASCADE;
diff --git a/src/test/regress/sql/pg16.sql b/src/test/regress/sql/pg16.sql
index 9cbdd7e47..c45f7d960 100644
--- a/src/test/regress/sql/pg16.sql
+++ b/src/test/regress/sql/pg16.sql
@@ -565,6 +565,32 @@ SELECT PERCENTILE_DISC((2 > random_normal(stddev => 1, mean => 0))::int::numeric
 FROM dist_table
 LEFT JOIN ref_table ON TRUE;
 
+--
+-- PG16 added WITH ADMIN FALSE option to GRANT ROLE
+-- WITH ADMIN FALSE is the default, make sure we propagate correctly in Citus
+-- Relevant PG commit: https://github.com/postgres/postgres/commit/e3ce2de
+--
+
+CREATE ROLE role1;
+CREATE ROLE role2;
+
+SET citus.log_remote_commands TO on;
+SET citus.grep_remote_commands = '%GRANT%';
+-- default admin option is false
+GRANT role1 TO role2;
+REVOKE role1 FROM role2;
+-- should behave same as default
+GRANT role1 TO role2 WITH ADMIN FALSE;
+REVOKE role1 FROM role2;
+-- with admin option and with admin true are the same
+GRANT role1 TO role2 WITH ADMIN OPTION;
+REVOKE role1 FROM role2;
+GRANT role1 TO role2 WITH ADMIN TRUE;
+REVOKE role1 FROM role2;
+
+RESET citus.log_remote_commands;
+RESET citus.grep_remote_commands;
+
 \set VERBOSITY terse
 SET client_min_messages TO ERROR;
 DROP EXTENSION postgres_fdw CASCADE;