Fix formatting and add comments

src/backend/distributed/commands/role.c

@@ -517,13 +517,14 @@ GenerateCreateOrAlterRoleCommand(Oid roleOid)
 {
 	HeapTuple roleTuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleOid));
 	Form_pg_authid role = ((Form_pg_authid) GETSTRUCT(roleTuple));
+	char *rolename = pstrdup(NameStr(role->rolname));
 
 	CreateRoleStmt *createRoleStmt = NULL;
 	if (EnableCreateRolePropagation)
 	{
 		createRoleStmt = makeNode(CreateRoleStmt);
 		createRoleStmt->stmt_type = ROLESTMT_ROLE;
-		createRoleStmt->role = pstrdup(NameStr(role->rolname));
+		createRoleStmt->role = rolename;
 		createRoleStmt->options = GenerateRoleOptionsList(roleTuple);
 	}
 
@@ -534,7 +535,7 @@ GenerateCreateOrAlterRoleCommand(Oid roleOid)
 		alterRoleStmt->role = makeNode(RoleSpec);
 		alterRoleStmt->role->roletype = ROLESPEC_CSTRING;
 		alterRoleStmt->role->location = -1;
-		alterRoleStmt->role->rolename = pstrdup(NameStr(role->rolname));
+		alterRoleStmt->role->rolename = rolename;
 		alterRoleStmt->action = 1;
 		alterRoleStmt->options = GenerateRoleOptionsList(roleTuple);
 	}
@@ -546,7 +547,7 @@ GenerateCreateOrAlterRoleCommand(Oid roleOid)
 	{
 		/* add a worker_create_or_alter_role command if any of them are set */
 		char *createOrAlterRoleQuery = CreateCreateOrAlterRoleCommand(
-			pstrdup(NameStr(role->rolname)),
+			rolename,
 			createRoleStmt,
 			alterRoleStmt);
 
@@ -569,10 +570,8 @@ GenerateCreateOrAlterRoleCommand(Oid roleOid)
 			completeRoleList = lappend(completeRoleList, DeparseTreeNode(stmt));
 		}
 
-		List *secLabelOnRoleStmts = GenerateSecLabelOnRoleStmts(roleOid, pstrdup(NameStr(
-																					 role
-																					 ->
-																					 rolname)));
+		/* append SECURITY LABEL ON ROLE commands fot this specific user */
+		List *secLabelOnRoleStmts = GenerateSecLabelOnRoleStmts(roleOid, rolename);
 		stmt = NULL;
 		foreach_ptr(stmt, secLabelOnRoleStmts)
 		{

src/backend/distributed/commands/seclabel.c

@@ -23,6 +23,11 @@
 
 PG_FUNCTION_INFO_V1(citus_test_register_label_provider);
 
+
+/*
+ * citus_test_register_label_provider registers a dummy label provider
+ * named 'citus_tests_label_provider'. This is aimed to be used for testing.
+ */
 Datum
 citus_test_register_label_provider(PG_FUNCTION_ARGS)
 {
@@ -84,7 +89,9 @@ PreprocessSecLabelStmt(Node *node, const char *queryString,
 
 
 /*
- * PostprocessSecLabelStmt
+ * PostprocessSecLabelStmt ensures that all object dependencies exist on all
+ * nodes for the object in the SecLabelStmt. Currently, we only support SecLabelStmts
+ * operating on a ROLE object.
  */
 List *
 PostprocessSecLabelStmt(Node *node, const char *queryString)
@@ -112,7 +119,11 @@ PostprocessSecLabelStmt(Node *node, const char *queryString)
 
 
 /*
- * SecLabelStmtObjectAddress
+ * SecLabelStmtObjectAddress returns the object address of the object on
+ * which this statement operates (secLabelStmt->object). Note that it has no limitation
+ * on the object type being OBJECT_ROLE. This is intentionally implemented like this
+ * since it is fairly simple to implement and we might extend SECURITY LABEL propagation
+ * in the future to include more object types.
  */
 List *
 SecLabelStmtObjectAddress(Node *node, bool missing_ok, bool isPostprocess)
@@ -131,7 +142,8 @@ SecLabelStmtObjectAddress(Node *node, bool missing_ok, bool isPostprocess)
 
 
 /*
- * citus_test_object_relabel
+ * citus_test_object_relabel is a dummy function for check_object_relabel_type hook.
+ * It is meant to be used in tests combined with citus_test_register_label_provider
  */
 void
 citus_test_object_relabel(const ObjectAddress *object, const char *seclabel)

src/test/regress/expected/seclabel.out

@@ -1,9 +1,17 @@
+--
+-- SECLABEL
+--
+-- Test suite for SECURITY LABEL ON ROLE statements
+--
+-- first we remove one of the worker nodes to be able to test
+-- citus_add_node later
 SELECT citus_remove_node('localhost', :worker_2_port);
  citus_remove_node
 ---------------------------------------------------------------------
 
 (1 row)
 
+-- now we register a label provider
 CREATE FUNCTION citus_test_register_label_provider()
   RETURNS void
   LANGUAGE C
@@ -15,6 +23,10 @@ SELECT citus_test_register_label_provider();
 (1 row)
 
 CREATE ROLE user1;
+-- the registered label provider is per session only
+-- this means that we need to maintain the same connection to the worker node
+-- in order for the label provider to be visible there
+-- hence here we create the necessary session_level_connection_to_node functions
 SET citus.enable_metadata_sync TO off;
 CREATE OR REPLACE FUNCTION start_session_level_connection_to_node(text, integer)
         RETURNS void
@@ -42,12 +54,14 @@ CREATE OR REPLACE FUNCTION stop_session_level_connection_to_node()
         LANGUAGE C STRICT VOLATILE
         AS 'citus', $$stop_session_level_connection_to_node$$;
 RESET citus.enable_metadata_sync;
+-- now we establish a connection to the worker node
 SELECT start_session_level_connection_to_node('localhost', :worker_1_port);
  start_session_level_connection_to_node
 ---------------------------------------------------------------------
 
 (1 row)
 
+-- with that same connection, we register the label provider in the worker node
 SELECT run_commands_on_session_level_connection_to_node('SELECT citus_test_register_label_provider()');
  run_commands_on_session_level_connection_to_node
 ---------------------------------------------------------------------
@@ -56,15 +70,21 @@ SELECT run_commands_on_session_level_connection_to_node('SELECT citus_test_regis
 
 SET citus.log_remote_commands TO on;
 SET citus.grep_remote_commands = '%SECURITY LABEL%';
+-- then we run a security label statement which will use the same connection to the worker node
+-- it should finish successfully
 SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus_classified';
 NOTICE:  issuing SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_classified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
+-- adding a new node will fail because the label provider is not there
+-- however, this is enough for testing as we can see that the SECURITY LABEL commands
+-- will be propagated when adding a new node
 SELECT 1 FROM citus_add_node('localhost', :worker_2_port);
 NOTICE:  issuing SELECT worker_create_or_alter_role('user1', 'CREATE ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''', 'ALTER ROLE user1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT NOLOGIN NOREPLICATION NOBYPASSRLS CONNECTION LIMIT -1 PASSWORD NULL VALID UNTIL ''infinity''');SECURITY LABEL FOR citus_tests_label_provider ON ROLE user1 IS 'citus_classified'
 DETAIL:  on server postgres@localhost:xxxxx connectionId: xxxxxxx
 WARNING:  security label provider "citus_tests_label_provider" is not loaded
 CONTEXT:  while executing command on localhost:xxxxx
 ERROR:  failure on connection marked as essential: localhost:xxxxx
+-- cleanup
 RESET citus.log_remote_commands;
 SELECT stop_session_level_connection_to_node();
  stop_session_level_connection_to_node

src/test/regress/sql/seclabel.sql

@@ -1,63 +1,74 @@
+--
+-- SECLABEL
+--
+-- Test suite for SECURITY LABEL ON ROLE statements
+--
+
+-- first we remove one of the worker nodes to be able to test
+-- citus_add_node later
 SELECT citus_remove_node('localhost', :worker_2_port);
 
+-- now we register a label provider
 CREATE FUNCTION citus_test_register_label_provider()
   RETURNS void
   LANGUAGE C
   AS 'citus', $$citus_test_register_label_provider$$;
-
 SELECT citus_test_register_label_provider();
+
 CREATE ROLE user1;
 
-SET citus.enable_metadata_sync TO off;
+-- the registered label provider is per session only
+-- this means that we need to maintain the same connection to the worker node
+-- in order for the label provider to be visible there
+-- hence here we create the necessary session_level_connection_to_node functions
 
+SET citus.enable_metadata_sync TO off;
 CREATE OR REPLACE FUNCTION start_session_level_connection_to_node(text, integer)
         RETURNS void
         LANGUAGE C STRICT VOLATILE
         AS 'citus', $$start_session_level_connection_to_node$$;
-
 CREATE OR REPLACE FUNCTION override_backend_data_gpid(bigint)
         RETURNS void
         LANGUAGE C STRICT IMMUTABLE
         AS 'citus', $$override_backend_data_gpid$$;
-
 SELECT run_command_on_workers($$SET citus.enable_metadata_sync TO off;CREATE OR REPLACE FUNCTION override_backend_data_gpid(bigint)
         RETURNS void
         LANGUAGE C STRICT IMMUTABLE
         AS 'citus'$$);
-
 CREATE OR REPLACE FUNCTION run_commands_on_session_level_connection_to_node(text)
         RETURNS void
         LANGUAGE C STRICT VOLATILE
         AS 'citus', $$run_commands_on_session_level_connection_to_node$$;
-
 CREATE OR REPLACE FUNCTION stop_session_level_connection_to_node()
         RETURNS void
         LANGUAGE C STRICT VOLATILE
         AS 'citus', $$stop_session_level_connection_to_node$$;
-
 RESET citus.enable_metadata_sync;
 
+-- now we establish a connection to the worker node
 SELECT start_session_level_connection_to_node('localhost', :worker_1_port);
 
+-- with that same connection, we register the label provider in the worker node
 SELECT run_commands_on_session_level_connection_to_node('SELECT citus_test_register_label_provider()');
 
 SET citus.log_remote_commands TO on;
 SET citus.grep_remote_commands = '%SECURITY LABEL%';
 
+-- then we run a security label statement which will use the same connection to the worker node
+-- it should finish successfully
 SECURITY LABEL for citus_tests_label_provider ON ROLE user1 IS 'citus_classified';
 
+-- adding a new node will fail because the label provider is not there
+-- however, this is enough for testing as we can see that the SECURITY LABEL commands
+-- will be propagated when adding a new node
 SELECT 1 FROM citus_add_node('localhost', :worker_2_port);
 
+-- cleanup
 RESET citus.log_remote_commands;
-
 SELECT stop_session_level_connection_to_node();
-
 DROP FUNCTION stop_session_level_connection_to_node, run_commands_on_session_level_connection_to_node,
               override_backend_data_gpid, start_session_level_connection_to_node;
 SELECT run_command_on_workers($$ DROP FUNCTION override_backend_data_gpid $$);
-
 DROP FUNCTION citus_test_register_label_provider;
-
 DROP ROLE user1;
-
 SELECT 1 FROM citus_add_node('localhost', :worker_2_port);