From 09a7a336d13d977e42e57b016e12dc5932ba894b Mon Sep 17 00:00:00 2001 From: gurkanindibay Date: Mon, 25 Dec 2023 18:08:10 +0300 Subject: [PATCH] Fixes review comments --- src/backend/distributed/commands/database.c | 13 ++----- .../deparser/deparse_database_stmts.c | 32 +++++++++-------- .../expected/alter_database_propagation.out | 36 +++++++++---------- .../sql/alter_database_propagation.sql | 14 ++++---- 4 files changed, 45 insertions(+), 50 deletions(-) diff --git a/src/backend/distributed/commands/database.c b/src/backend/distributed/commands/database.c index 708cf7ebe..5bd84fb9c 100644 --- a/src/backend/distributed/commands/database.c +++ b/src/backend/distributed/commands/database.c @@ -193,17 +193,8 @@ PreprocessGrantOnDatabaseStmt(Node *node, const char *queryString, /* - * IsSetTablespaceStatement checks if the provided ALTER DATABASE statement is a SET TABLESPACE statement. - * - * This function takes a Node pointer representing a AlterDatabaseStmt, and checks - * if it is a SET TABLESPACE statement, which is used to move a table to a new - * tablespace. - * - * Parameters: - * stmt: A pointer to a Node representing AlterDatabaseStmt. - * - * Returns: - * true if the statement is a SET TABLESPACE statement, false otherwise. + * IsSetTablespaceStatement returns true if the statement is a SET TABLESPACE statement, + * false otherwise. */ static bool IsSetTablespaceStatement(AlterDatabaseStmt *stmt) diff --git a/src/backend/distributed/deparser/deparse_database_stmts.c b/src/backend/distributed/deparser/deparse_database_stmts.c index 10d9a87da..30ac3f32c 100644 --- a/src/backend/distributed/deparser/deparse_database_stmts.c +++ b/src/backend/distributed/deparser/deparse_database_stmts.c @@ -124,17 +124,27 @@ AppendGrantOnDatabaseStmt(StringInfo buf, GrantStmt *stmt) static void AppendAlterDatabaseStmt(StringInfo buf, AlterDatabaseStmt *stmt) { + if (list_length(stmt->options) == 0) + { + elog(ERROR, "got unexpected number of options for ALTER DATABASE"); + } + if (stmt->options) { DefElem *firstOption = linitial(stmt->options); if (strcmp(firstOption->defname, "tablespace") == 0) { AppendAlterDatabaseSetTablespace(buf, firstOption, stmt->dbname); + + /* SET tablespace cannot be combined with other options */ + return; } - else - { - AppendBasicAlterDatabaseOptions(buf, stmt); - } + + + appendStringInfo(buf, "ALTER DATABASE %s WITH", + quote_identifier(stmt->dbname)); + + AppendBasicAlterDatabaseOptions(buf, stmt); } appendStringInfo(buf, ";"); @@ -154,7 +164,7 @@ AppendAlterDatabaseSetTablespace(StringInfo buf, DefElem *def, char *dbname) * AppendBasicAlterDatabaseOptions appends basic ALTER DATABASE options to a string buffer. * Basic options are those that can be appended to the ALTER DATABASE statement * after the "WITH" keyword.(i.e. ALLOW_CONNECTIONS, CONNECTION LIMIT, IS_TEMPLATE) - * The tablespace option is not a basic option since it is defined with SET option. + * For example, the tablespace option is not a basic option since it is defined via SET keyword. * * This function takes a string buffer and an AlterDatabaseStmt as input. * It appends the basic options to the string buffer. @@ -163,17 +173,9 @@ AppendAlterDatabaseSetTablespace(StringInfo buf, DefElem *def, char *dbname) static void AppendBasicAlterDatabaseOptions(StringInfo buf, AlterDatabaseStmt *stmt) { - ListCell *cell = NULL; - bool prefixAppendedForBasicOptions = false; - foreach(cell, stmt->options) + DefElem *def = NULL; + foreach_ptr(def, stmt->options) { - DefElem *def = castNode(DefElem, lfirst(cell)); - if (!prefixAppendedForBasicOptions) - { - appendStringInfo(buf, "ALTER DATABASE %s WITH", quote_identifier( - stmt->dbname)); - prefixAppendedForBasicOptions = true; - } DefElemOptionToStatement(buf, def, alterDatabaseOptionFormats, lengthof( alterDatabaseOptionFormats)); } diff --git a/src/test/regress/expected/alter_database_propagation.out b/src/test/regress/expected/alter_database_propagation.out index 7939ce899..ffb5674f8 100644 --- a/src/test/regress/expected/alter_database_propagation.out +++ b/src/test/regress/expected/alter_database_propagation.out @@ -140,21 +140,21 @@ DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx NOTICE: issuing ALTER DATABASE regression RESET lock_timeout DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx set citus.enable_create_database_propagation=on; -create database regression2; -alter database regression2 with CONNECTION LIMIT 100; -NOTICE: issuing ALTER DATABASE regression2 WITH CONNECTION LIMIT 100; +create database "regression!'2"; +alter database "regression!'2" with CONNECTION LIMIT 100; +NOTICE: issuing ALTER DATABASE "regression!'2" WITH CONNECTION LIMIT 100; DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing ALTER DATABASE regression2 WITH CONNECTION LIMIT 100; +NOTICE: issuing ALTER DATABASE "regression!'2" WITH CONNECTION LIMIT 100; DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -alter database regression2 with IS_TEMPLATE true CONNECTION LIMIT 50; -NOTICE: issuing ALTER DATABASE regression2 WITH IS_TEMPLATE true CONNECTION LIMIT 50; +alter database "regression!'2" with IS_TEMPLATE true CONNECTION LIMIT 50; +NOTICE: issuing ALTER DATABASE "regression!'2" WITH IS_TEMPLATE true CONNECTION LIMIT 50; DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing ALTER DATABASE regression2 WITH IS_TEMPLATE true CONNECTION LIMIT 50; +NOTICE: issuing ALTER DATABASE "regression!'2" WITH IS_TEMPLATE true CONNECTION LIMIT 50; DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -alter database regression2 with IS_TEMPLATE false; -NOTICE: issuing ALTER DATABASE regression2 WITH IS_TEMPLATE false; +alter database "regression!'2" with IS_TEMPLATE false; +NOTICE: issuing ALTER DATABASE "regression!'2" WITH IS_TEMPLATE false; DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing ALTER DATABASE regression2 WITH IS_TEMPLATE false; +NOTICE: issuing ALTER DATABASE "regression!'2" WITH IS_TEMPLATE false; DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx \set alter_db_tablespace :abs_srcdir '/tmp_check/ts3' CREATE TABLESPACE alter_db_tablespace LOCATION :'alter_db_tablespace'; @@ -167,16 +167,16 @@ CREATE TABLESPACE alter_db_tablespace LOCATION :'alter_db_tablespace'; \c - - - :master_port set citus.log_remote_commands = true; set citus.grep_remote_commands = '%ALTER DATABASE%'; -alter database regression2 set TABLESPACE alter_db_tablespace; -NOTICE: issuing ALTER DATABASE regression2 SET TABLESPACE alter_db_tablespace; +alter database "regression!'2" set TABLESPACE alter_db_tablespace; +NOTICE: issuing ALTER DATABASE "regression!'2" SET TABLESPACE alter_db_tablespace DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing ALTER DATABASE regression2 SET TABLESPACE alter_db_tablespace; +NOTICE: issuing ALTER DATABASE "regression!'2" SET TABLESPACE alter_db_tablespace DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx set citus.enable_create_database_propagation=on; -alter database regression2 rename to regression3; -NOTICE: issuing ALTER DATABASE regression2 RENAME TO regression3 +alter database "regression!'2" rename to regression3; +NOTICE: issuing ALTER DATABASE "regression!'2" RENAME TO regression3 DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing ALTER DATABASE regression2 RENAME TO regression3 +NOTICE: issuing ALTER DATABASE "regression!'2" RENAME TO regression3 DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -- check that the local database rename and alter comnmand is not propagated set citus.enable_create_database_propagation=off; @@ -203,9 +203,9 @@ SELECT result FROM run_command_on_all_nodes( (3 rows) alter database regression4 set TABLESPACE "ts-needs\!escape"; -NOTICE: issuing ALTER DATABASE regression4 SET TABLESPACE "ts-needs\!escape"; +NOTICE: issuing ALTER DATABASE regression4 SET TABLESPACE "ts-needs\!escape" DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -NOTICE: issuing ALTER DATABASE regression4 SET TABLESPACE "ts-needs\!escape"; +NOTICE: issuing ALTER DATABASE regression4 SET TABLESPACE "ts-needs\!escape" DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx drop database regression4; set citus.log_remote_commands = false; diff --git a/src/test/regress/sql/alter_database_propagation.sql b/src/test/regress/sql/alter_database_propagation.sql index 52602b370..35a4c5479 100644 --- a/src/test/regress/sql/alter_database_propagation.sql +++ b/src/test/regress/sql/alter_database_propagation.sql @@ -49,10 +49,12 @@ alter database regression set lock_timeout to DEFAULT; alter database regression RESET lock_timeout; set citus.enable_create_database_propagation=on; -create database regression2; -alter database regression2 with CONNECTION LIMIT 100; -alter database regression2 with IS_TEMPLATE true CONNECTION LIMIT 50; -alter database regression2 with IS_TEMPLATE false; +create database "regression!'2"; +alter database "regression!'2" with CONNECTION LIMIT 100; +alter database "regression!'2" with IS_TEMPLATE true CONNECTION LIMIT 50; +alter database "regression!'2" with IS_TEMPLATE false; + + \set alter_db_tablespace :abs_srcdir '/tmp_check/ts3' @@ -71,10 +73,10 @@ CREATE TABLESPACE alter_db_tablespace LOCATION :'alter_db_tablespace'; set citus.log_remote_commands = true; set citus.grep_remote_commands = '%ALTER DATABASE%'; -alter database regression2 set TABLESPACE alter_db_tablespace; +alter database "regression!'2" set TABLESPACE alter_db_tablespace; set citus.enable_create_database_propagation=on; -alter database regression2 rename to regression3; +alter database "regression!'2" rename to regression3; -- check that the local database rename and alter comnmand is not propagated set citus.enable_create_database_propagation=off;