Address reviews

src/backend/distributed/commands/distribute_object_ops.c

@@ -377,7 +377,7 @@ static DistributeObjectOps Any_Rename = {
 static DistributeObjectOps Any_SecLabel = {
 	.deparse = DeparseSecLabelStmt,
 	.qualify = NULL,
-	.preprocess = PreprocessSecLabelStmt,
+	.preprocess = NULL,
 	.postprocess = PostprocessSecLabelStmt,
 	.operationType = DIST_OPS_ALTER,
 	.address = SecLabelStmtObjectAddress,

src/backend/distributed/commands/role.c

@@ -937,7 +937,7 @@ GenerateSecLabelOnRoleStmts(Oid roleid, char *rolename)
 	{
 		SecLabelStmt *secLabelStmt = makeNode(SecLabelStmt);
 		secLabelStmt->objtype = OBJECT_ROLE;
-		secLabelStmt->object = (Node *) makeString(rolename);
+		secLabelStmt->object = (Node *) makeString(pstrdup(rolename));
 
 		Datum datumArray[Natts_pg_shseclabel];
 		bool isNullArray[Natts_pg_shseclabel];

src/backend/distributed/commands/seclabel.c

@@ -19,18 +19,17 @@
 #include "distributed/metadata_sync.h"
 #include "distributed/metadata/distobject.h"
 
+
 /*
- * PreprocessSecLabelStmt is executed before the statement is applied to the local
- * postgres instance.
- *
- * In this stage we can prepare the commands that need to be run on all workers to assign
+ * PostprocessSecLabelStmt prepares the commands that need to be run on all workers to assign
  * security labels on distributed objects, currently supporting just Role objects.
+ * It also ensures that all object dependencies exist on all
+ * nodes for the object in the SecLabelStmt.
  */
 List *
-PreprocessSecLabelStmt(Node *node, const char *queryString,
-					   ProcessUtilityContext processUtilityContext)
+PostprocessSecLabelStmt(Node *node, const char *queryString)
 {
-	if (!IsCoordinator() || !ShouldPropagate())
+	if (!ShouldPropagate())
 	{
 		return NIL;
 	}
@@ -45,13 +44,17 @@ PreprocessSecLabelStmt(Node *node, const char *queryString,
 
 	if (secLabelStmt->objtype != OBJECT_ROLE)
 	{
-		if (EnableUnsupportedFeatureMessages)
+		/*
+		 * If we are not in the coordinator, we don't want to interrupt the security
+		 * label command with notices, the user expects that from the worker node
+		 * the command will not be propagated
+		 */
+		if (EnableUnsupportedFeatureMessages && IsCoordinator())
 		{
 			ereport(NOTICE, (errmsg("not propagating SECURITY LABEL commands whose "
 									"object type is not role"),
 							 errhint("Connect to worker nodes directly to manually "
-									 "run the same SECURITY LABEL command after "
-									 "disabling DDL propagation.")));
+									 "run the same SECURITY LABEL command.")));
 		}
 		return NIL;
 	}
@@ -61,6 +64,9 @@ PreprocessSecLabelStmt(Node *node, const char *queryString,
 		return NIL;
 	}
 
+	EnsureCoordinator();
+	EnsureAllObjectDependenciesExistOnAllNodes(objectAddresses);
+
 	const char *sql = DeparseTreeNode((Node *) secLabelStmt);
 
 	List *commandList = list_make3(DISABLE_DDL_PROPAGATION,
@@ -71,36 +77,6 @@ PreprocessSecLabelStmt(Node *node, const char *queryString,
 }
 
 
-/*
- * 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)
-{
-	if (!EnableCreateRolePropagation || !IsCoordinator() || !ShouldPropagate())
-	{
-		return NIL;
-	}
-
-	SecLabelStmt *secLabelStmt = castNode(SecLabelStmt, node);
-
-	if (secLabelStmt->objtype != OBJECT_ROLE)
-	{
-		return NIL;
-	}
-
-	List *objectAddresses = GetObjectAddressListFromParseTree(node, false, false);
-	if (IsAnyObjectDistributed(objectAddresses))
-	{
-		EnsureAllObjectDependenciesExistOnAllNodes(objectAddresses);
-	}
-
-	return NIL;
-}
-
-
 /*
  * SecLabelStmtObjectAddress returns the object address of the object on
  * which this statement operates (secLabelStmt->object). Note that it has no limitation

src/backend/distributed/deparser/deparse_seclabel_stmts.c

@@ -44,7 +44,7 @@ AppendSecLabelStmt(StringInfo buf, SecLabelStmt *stmt)
 
 	if (stmt->provider != NULL)
 	{
-		appendStringInfo(buf, "FOR %s ", stmt->provider);
+		appendStringInfo(buf, "FOR %s ", quote_identifier(stmt->provider));
 	}
 
 	appendStringInfoString(buf, "ON ");
@@ -53,7 +53,7 @@ AppendSecLabelStmt(StringInfo buf, SecLabelStmt *stmt)
 	{
 		case OBJECT_ROLE:
 		{
-			appendStringInfo(buf, "ROLE %s ", strVal(stmt->object));
+			appendStringInfo(buf, "ROLE %s ", quote_identifier(strVal(stmt->object)));
 			break;
 		}
 

src/include/distributed/commands.h

@@ -522,8 +522,6 @@ extern List * AlterSchemaRenameStmtObjectAddress(Node *node, bool missing_ok, bo
 												 isPostprocess);
 
 /* seclabel.c - forward declarations*/
-extern List * PreprocessSecLabelStmt(Node *node, const char *queryString,
-									 ProcessUtilityContext processUtilityContext);
 extern List * PostprocessSecLabelStmt(Node *node, const char *queryString);
 extern List * SecLabelStmtObjectAddress(Node *node, bool missing_ok, bool isPostprocess);
 extern void citus_test_object_relabel(const ObjectAddress *object, const char *seclabel);

src/test/regress/Makefile

@@ -101,7 +101,7 @@ check-base: all
 
 # check-minimal only sets up the cluster
 check-minimal: all
-	$(pg_regress_multi_check) --load-extension=citus --seclabel \
+	$(pg_regress_multi_check) --load-extension=citus \
 	-- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/minimal_schedule $(EXTRA_TESTS)
 
 check-base-vg: all
@@ -118,7 +118,7 @@ check-minimal-mx: all
 	-- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/mx_minimal_schedule $(EXTRA_TESTS)
 
 check-custom-schedule: all
-	$(pg_regress_multi_check) --load-extension=citus --seclabel-test --worker-count=$(WORKERCOUNT) \
+	$(pg_regress_multi_check) --load-extension=citus --worker-count=$(WORKERCOUNT) \
 	-- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/$(SCHEDULE) $(EXTRA_TESTS)
 
 check-failure-custom-schedule: all
@@ -159,7 +159,7 @@ check-enterprise: all
 	$(pg_regress_multi_check) --load-extension=citus \
 	-- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/enterprise_schedule $(EXTRA_TESTS)
 check-multi-1: all
-	$(pg_regress_multi_check) --load-extension=citus --seclabel-test \
+	$(pg_regress_multi_check) --load-extension=citus \
 	-- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/multi_1_schedule $(EXTRA_TESTS)
 
 check-multi-hyperscale: all