Fixes review notes

src/backend/distributed/commands/role.c

@@ -1347,7 +1347,7 @@ CreateRoleStmtObjectAddress(Node *node, bool missing_ok, bool isPostprocess)
  * with the role the role needs to be visible on all connections used by the transaction,
  * meaning we can only use 1 connection per node.
  */
-void
+static void
 EnsureSequentialModeForRoleDDL(void)
 {
 	if (!IsTransactionBlock())

src/backend/distributed/deparser/deparse_database_stmts.c

@@ -34,6 +34,10 @@ static void AppendAlterDatabaseStmt(StringInfo buf, AlterDatabaseStmt *stmt);
 static void AppendCreateDatabaseStmt(StringInfo buf, CreatedbStmt *stmt);
 static void AppendDropDatabaseStmt(StringInfo buf, DropdbStmt *stmt);
 static void AppendGrantOnDatabaseStmt(StringInfo buf, GrantStmt *stmt);
+static void AppendBasicAlterDatabaseOptions(StringInfo buf, DefElem *def, bool *
+											prefixAppendedForBasicOptions, char *dbname);
+static void AppendGrantDatabases(StringInfo buf, GrantStmt *stmt);
+static void AppendAlterDatabaseSetTablespace(StringInfo buf, DefElem *def, char *dbname);
 
 const DefElemOptionFormat create_database_option_formats[] = {
 	{ "owner", " OWNER %s", OPTION_FORMAT_STRING },
@@ -114,6 +118,34 @@ AppendAlterDatabaseOwnerStmt(StringInfo buf, AlterOwnerStmt *stmt)
 }
 
 
+char *
+DeparseGrantOnDatabaseStmt(Node *node)
+{
+	GrantStmt *stmt = castNode(GrantStmt, node);
+	Assert(stmt->objtype == OBJECT_DATABASE);
+
+	StringInfoData str = { 0 };
+	initStringInfo(&str);
+
+	AppendGrantOnDatabaseStmt(&str, stmt);
+
+	return str.data;
+}
+
+
+static void
+AppendGrantOnDatabaseStmt(StringInfo buf, GrantStmt *stmt)
+{
+	Assert(stmt->objtype == OBJECT_DATABASE);
+
+	AppendGrantSharedPrefix(buf, stmt);
+
+	AppendGrantDatabases(buf, stmt);
+
+	AppendGrantSharedSuffix(buf, stmt);
+}
+
+
 static void
 AppendGrantDatabases(StringInfo buf, GrantStmt *stmt)
 {
@@ -132,40 +164,17 @@ AppendGrantDatabases(StringInfo buf, GrantStmt *stmt)
 }
 
 
-static void
+char *
-AppendGrantOnDatabaseStmt(StringInfo buf, GrantStmt *stmt)
+DeparseAlterDatabaseStmt(Node *node)
 {
-	Assert(stmt->objtype == OBJECT_DATABASE);
+	AlterDatabaseStmt *stmt = castNode(AlterDatabaseStmt, node);
 
-	AppendGrantSharedPrefix(buf, stmt);
+	StringInfoData str = { 0 };
+	initStringInfo(&str);
 
-	AppendGrantDatabases(buf, stmt);
+	AppendAlterDatabaseStmt(&str, stmt);
 
-	AppendGrantSharedSuffix(buf, stmt);
+	return str.data;
-}
-
-
-static bool
-AppendBasicAlterDatabaseOptions(StringInfo buf, DefElem *def, bool
-								prefixAppendedForBasicOptions, char *dbname)
-{
-	if (!prefixAppendedForBasicOptions)
-	{
-		appendStringInfo(buf, "ALTER DATABASE %s WITH", quote_identifier(dbname));
-		prefixAppendedForBasicOptions = true;
-	}
-	DefElemOptionToStatement(buf, def, alterDatabaseOptionFormats, lengthof(
-								 alterDatabaseOptionFormats));
-	return prefixAppendedForBasicOptions;
-}
-
-
-static void
-AppendAlterDatabaseSetTablespace(StringInfo buf, DefElem *def, char *dbname)
-{
-	appendStringInfo(buf,
-					 "ALTER DATABASE %s SET TABLESPACE %s",
-					 quote_identifier(dbname), quote_identifier(defGetString(def)));
 }
 
 
@@ -186,11 +195,8 @@ AppendAlterDatabaseStmt(StringInfo buf, AlterDatabaseStmt *stmt)
 			}
 			else
 			{
-				prefixAppendedForBasicOptions = AppendBasicAlterDatabaseOptions(buf,
+				AppendBasicAlterDatabaseOptions(buf, def, &prefixAppendedForBasicOptions,
-																				def,
+												stmt->dbname);
-																				prefixAppendedForBasicOptions,
-																				stmt->
-																				dbname);
 			}
 		}
 	}
@@ -199,32 +205,40 @@ AppendAlterDatabaseStmt(StringInfo buf, AlterDatabaseStmt *stmt)
 }
 
 
-char *
+static void
-DeparseGrantOnDatabaseStmt(Node *node)
+AppendAlterDatabaseSetTablespace(StringInfo buf, DefElem *def, char *dbname)
 {
-	GrantStmt *stmt = castNode(GrantStmt, node);
+	appendStringInfo(buf,
-	Assert(stmt->objtype == OBJECT_DATABASE);
+					 "ALTER DATABASE %s SET TABLESPACE %s",
-
+					 quote_identifier(dbname), quote_identifier(defGetString(def)));
-	StringInfoData str = { 0 };
-	initStringInfo(&str);
-
-	AppendGrantOnDatabaseStmt(&str, stmt);
-
-	return str.data;
 }
 
 
-char *
+/*
-DeparseAlterDatabaseStmt(Node *node)
+ * Appends basic ALTER DATABASE options to a string buffer.
+ *
+ * This function takes a string buffer, a DefElem representing a database option,
+ * a boolean indicating whether the prefix "ALTER DATABASE <dbname> WITH" has
+ * already been appended, and a database name. It appends the SQL representation
+ * of the database option to the string buffer.
+ *
+ * Returns:
+ *   A boolean indicating whether the prefix "ALTER DATABASE <dbname> WITH" has
+ *   been appended to the buffer. This is the same as the input
+ *   prefixAppendedForBasicOptions if the prefix was already appended, or true
+ *   if this function appended the prefix.
+ */
+static void
+AppendBasicAlterDatabaseOptions(StringInfo buf, DefElem *def, bool *
+								prefixAppendedForBasicOptions, char *dbname)
 {
-	AlterDatabaseStmt *stmt = castNode(AlterDatabaseStmt, node);
+	if (!(*prefixAppendedForBasicOptions))
-
+	{
-	StringInfoData str = { 0 };
+		appendStringInfo(buf, "ALTER DATABASE %s WITH", quote_identifier(dbname));
-	initStringInfo(&str);
+		*prefixAppendedForBasicOptions = true;
-
+	}
-	AppendAlterDatabaseStmt(&str, stmt);
+	DefElemOptionToStatement(buf, def, alterDatabaseOptionFormats, lengthof(
-
+								 alterDatabaseOptionFormats));
-	return str.data;
 }
 
 
@@ -278,6 +292,20 @@ DeparseAlterDatabaseRenameStmt(Node *node)
 }
 
 
+char *
+DeparseAlterDatabaseSetStmt(Node *node)
+{
+	AlterDatabaseSetStmt *stmt = castNode(AlterDatabaseSetStmt, node);
+
+	StringInfoData str = { 0 };
+	initStringInfo(&str);
+
+	AppendAlterDatabaseSetStmt(&str, stmt);
+
+	return str.data;
+}
+
+
 static void
 AppendAlterDatabaseSetStmt(StringInfo buf, AlterDatabaseSetStmt *stmt)
 {
@@ -290,14 +318,13 @@ AppendAlterDatabaseSetStmt(StringInfo buf, AlterDatabaseSetStmt *stmt)
 
 
 char *
-DeparseAlterDatabaseSetStmt(Node *node)
+DeparseCreateDatabaseStmt(Node *node)
 {
-	AlterDatabaseSetStmt *stmt = castNode(AlterDatabaseSetStmt, node);
+	CreatedbStmt *stmt = castNode(CreatedbStmt, node);
-
 	StringInfoData str = { 0 };
 	initStringInfo(&str);
 
-	AppendAlterDatabaseSetStmt(&str, stmt);
+	AppendCreateDatabaseStmt(&str, stmt);
 
 	return str.data;
 }
@@ -326,13 +353,13 @@ AppendCreateDatabaseStmt(StringInfo buf, CreatedbStmt *stmt)
 
 
 char *
-DeparseCreateDatabaseStmt(Node *node)
+DeparseDropDatabaseStmt(Node *node)
 {
-	CreatedbStmt *stmt = castNode(CreatedbStmt, node);
+	DropdbStmt *stmt = castNode(DropdbStmt, node);
 	StringInfoData str = { 0 };
 	initStringInfo(&str);
 
-	AppendCreateDatabaseStmt(&str, stmt);
+	AppendDropDatabaseStmt(&str, stmt);
 
 	return str.data;
 }
@@ -372,16 +399,3 @@ AppendDropDatabaseStmt(StringInfo buf, DropdbStmt *stmt)
 		appendStringInfo(buf, " )");
 	}
 }
-
-
-char *
-DeparseDropDatabaseStmt(Node *node)
-{
-	DropdbStmt *stmt = castNode(DropdbStmt, node);
-	StringInfoData str = { 0 };
-	initStringInfo(&str);
-
-	AppendDropDatabaseStmt(&str, stmt);
-
-	return str.data;
-}

src/test/regress/sql/create_drop_database_propagation.sql

@@ -129,7 +129,6 @@ CREATE USER "role-needs\!escape";
 CREATE DATABASE "db-needs\!escape" owner "role-needs\!escape" tablespace "ts-needs\!escape";
 
 -- Rename it to make check_database_on_all_nodes happy.
--- Today we don't support ALTER DATABASE .. RENAME TO .., so need to propagate it manually.
 
 ALTER DATABASE "db-needs\!escape" RENAME TO db_needs_escape;
 SELECT * FROM public.check_database_on_all_nodes('db_needs_escape') ORDER BY node_type;