From 1866030bb78a386c64956d6d01fc9390b3154af7 Mon Sep 17 00:00:00 2001 From: Tuan Pham Anh Date: Mon, 24 Mar 2025 15:34:11 +0000 Subject: [PATCH] Merge pull request #75622 from NamHoaiNguyen/alter_database_comment_issue_73351 Allow altering database comment --- .../statements/alter/database-comment.md | 55 +++++++++++++++++ src/Access/Common/AccessType.h | 1 + src/Databases/DatabaseMemory.cpp | 5 ++ src/Databases/DatabaseMemory.h | 2 + src/Databases/DatabaseOnDisk.cpp | 6 ++ src/Databases/DatabaseOnDisk.h | 4 ++ src/Databases/DatabasesCommon.cpp | 20 ++++++ src/Databases/DatabasesCommon.h | 2 + src/Databases/IDatabase.cpp | 5 ++ src/Databases/IDatabase.h | 5 ++ src/Databases/MySQL/DatabaseMySQL.cpp | 7 +++ src/Databases/MySQL/DatabaseMySQL.h | 3 + .../PostgreSQL/DatabasePostgreSQL.cpp | 6 ++ src/Databases/PostgreSQL/DatabasePostgreSQL.h | 3 + src/Databases/SQLite/DatabaseSQLite.cpp | 6 ++ src/Databases/SQLite/DatabaseSQLite.h | 4 ++ src/Interpreters/DatabaseCatalog.cpp | 43 +++++++++++++ src/Interpreters/DatabaseCatalog.h | 2 + src/Interpreters/InterpreterAlterQuery.cpp | 24 ++++++-- src/Parsers/ASTAlterQuery.cpp | 2 +- src/Parsers/ASTAlterQuery.h | 2 + src/Parsers/CommonParsers.h | 1 + src/Parsers/ParserAlterQuery.cpp | 7 +++ src/Storages/AlterCommands.cpp | 9 +++ src/Storages/AlterCommands.h | 1 + .../test_mysql_database_engine/test.py | 36 +++++++++++ .../test_postgresql_database_engine/test.py | 23 +++++++ .../01271_show_privileges.reference | 1 + ...28_alter_database_modify_comment.reference | 52 ++++++++++++++++ .../03328_alter_database_modify_comment.sh | 61 +++++++++++++++++++ ...329_grant_alter_database_comment.reference | 4 ++ .../03329_grant_alter_database_comment.sh | 37 +++++++++++ 32 files changed, 432 insertions(+), 7 deletions(-) create mode 100644 docs/en/sql-reference/statements/alter/database-comment.md create mode 100644 tests/queries/0_stateless/03328_alter_database_modify_comment.reference create mode 100755 tests/queries/0_stateless/03328_alter_database_modify_comment.sh create mode 100644 tests/queries/0_stateless/03329_grant_alter_database_comment.reference create mode 100755 tests/queries/0_stateless/03329_grant_alter_database_comment.sh diff --git a/docs/en/sql-reference/statements/alter/database-comment.md b/docs/en/sql-reference/statements/alter/database-comment.md new file mode 100644 index 000000000000..7208d2b8fe2a --- /dev/null +++ b/docs/en/sql-reference/statements/alter/database-comment.md @@ -0,0 +1,55 @@ +--- +description: 'Documentation for ALTER DATABASE ... MODIFY COMMENT Statements' +slug: /sql-reference/statements/alter/database-comment +sidebar_position: 51 +sidebar_label: 'UPDATE' +title: 'ALTER DATABASE ... MODIFY COMMENT Statements' +--- + +# ALTER DATABASE ... MODIFY COMMENT + +Adds, modifies, or removes comment to the table, regardless if it was set before or not. Comment change is reflected in both [system.databases](/operations/system-tables/databases.md) and `SHOW CREATE DATABASE` query. + +**Syntax** + +``` sql +ALTER DATABASE [db].name [ON CLUSTER cluster] MODIFY COMMENT 'Comment' +``` + +**Examples** + +Creating a DATABASE with comment (for more information, see the [COMMENT](/sql-reference/statements/create/table#comment-clause) clause): + +``` sql +CREATE DATABASE database_with_comment ENGINE = Memory COMMENT 'The temporary database'; +``` + +Modifying the table comment: + +``` sql +ALTER DATABASE database_with_comment MODIFY COMMENT 'new comment on a database'; +SELECT comment FROM system.databases WHERE name = 'database_with_comment'; +``` + +Output of a new comment: + +```text +┌─comment─────────────────┐ +│ new comment on database │ +└─────────────────────────┘ +``` + +Removing the database comment: + +``` sql +ALTER DATABASE database_with_comment MODIFY COMMENT ''; +SELECT comment FROM system.databases WHERE name = 'database_with_comment'; +``` + +Output of a removed comment: + +```text +┌─comment─┐ +│ │ +└─────────┘ +``` diff --git a/src/Access/Common/AccessType.h b/src/Access/Common/AccessType.h index 1139808ff334..ec64b2870e24 100644 --- a/src/Access/Common/AccessType.h +++ b/src/Access/Common/AccessType.h @@ -43,6 +43,7 @@ enum class AccessType : uint8_t M(ALTER_MATERIALIZE_COLUMN, "MATERIALIZE COLUMN", COLUMN, ALTER_COLUMN) \ M(ALTER_COLUMN, "", GROUP, ALTER_TABLE) /* allow to execute ALTER {ADD|DROP|MODIFY...} COLUMN */\ M(ALTER_MODIFY_COMMENT, "MODIFY COMMENT", TABLE, ALTER_TABLE) /* modify table comment */\ + M(ALTER_MODIFY_DATABASE_COMMENT, "MODIFY DATABASE COMMENT", DATABASE, ALTER_DATABASE) /* modify database comment*/\ \ M(ALTER_ORDER_BY, "ALTER MODIFY ORDER BY, MODIFY ORDER BY", TABLE, ALTER_INDEX) \ M(ALTER_SAMPLE_BY, "ALTER MODIFY SAMPLE BY, MODIFY SAMPLE BY", TABLE, ALTER_INDEX) \ diff --git a/src/Databases/DatabaseMemory.cpp b/src/Databases/DatabaseMemory.cpp index 28503eb7aefb..5fe74b11a6a3 100644 --- a/src/Databases/DatabaseMemory.cpp +++ b/src/Databases/DatabaseMemory.cpp @@ -213,6 +213,11 @@ std::vector> DatabaseMemory::getTablesForBackup(co return res; } +void DatabaseMemory::alterDatabaseComment(const AlterCommand & command) +{ + DB::updateDatabaseCommentWithMetadataFile(shared_from_this(), command); +} + void registerDatabaseMemory(DatabaseFactory & factory) { auto create_fn = [](const DatabaseFactory::Arguments & args) diff --git a/src/Databases/DatabaseMemory.h b/src/Databases/DatabaseMemory.h index 0f703a0b46e8..a4f0da21833b 100644 --- a/src/Databases/DatabaseMemory.h +++ b/src/Databases/DatabaseMemory.h @@ -52,6 +52,8 @@ class DatabaseMemory final : public DatabaseWithOwnTablesBase std::vector> getTablesForBackup(const FilterByNameFunction & filter, const ContextPtr & local_context) const override; + void alterDatabaseComment(const AlterCommand & alter_command) override; + private: void removeDataPath(ContextPtr local_context); diff --git a/src/Databases/DatabaseOnDisk.cpp b/src/Databases/DatabaseOnDisk.cpp index 89d90c7b1689..7ff436ec3347 100644 --- a/src/Databases/DatabaseOnDisk.cpp +++ b/src/Databases/DatabaseOnDisk.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -916,4 +917,9 @@ void DatabaseOnDisk::checkTableNameLengthUnlocked(const String & database_name_, } } +void DatabaseOnDisk::alterDatabaseComment(const AlterCommand & command) +{ + DB::updateDatabaseCommentWithMetadataFile(shared_from_this(), command); +} + } diff --git a/src/Databases/DatabaseOnDisk.h b/src/Databases/DatabaseOnDisk.h index cfcfe927bd20..91fac0b41121 100644 --- a/src/Databases/DatabaseOnDisk.h +++ b/src/Databases/DatabaseOnDisk.h @@ -12,6 +12,8 @@ namespace DB { class Context; +struct AlterCommand; + std::pair createTableFromAST( ASTCreateQuery ast_create_query, const String & database_name, @@ -82,6 +84,8 @@ class DatabaseOnDisk : public DatabaseWithOwnTablesBase void modifySettingsMetadata(const SettingsChanges & settings_changes, ContextPtr query_context); + void alterDatabaseComment(const AlterCommand & alter_command) override; + protected: static constexpr const char * create_suffix = ".tmp"; static constexpr const char * drop_suffix = ".tmp_drop"; diff --git a/src/Databases/DatabasesCommon.cpp b/src/Databases/DatabasesCommon.cpp index 663d9768a09c..3a4a20eb042a 100644 --- a/src/Databases/DatabasesCommon.cpp +++ b/src/Databases/DatabasesCommon.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -42,6 +43,7 @@ namespace ErrorCodes extern const int NOT_IMPLEMENTED; extern const int LOGICAL_ERROR; extern const int CANNOT_GET_CREATE_TABLE_QUERY; + extern const int BAD_ARGUMENTS; } namespace { @@ -324,6 +326,24 @@ void writeMetadataFile(std::shared_ptr db_disk, const String & file_path, out.reset(); } +void updateDatabaseCommentWithMetadataFile(DatabasePtr db, const AlterCommand & command) +{ + if (!command.comment) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Unable to obtain database comment from query"); + + String old_database_comment = db->getDatabaseComment(); + db->setDatabaseComment(command.comment.value()); + + try + { + DatabaseCatalog::instance().updateMetadataFile(db); + } + catch (...) + { + db->setDatabaseComment(old_database_comment); + throw; + } +} DatabaseWithOwnTablesBase::DatabaseWithOwnTablesBase(const String & name_, const String & logger, ContextPtr context_) : IDatabase(name_), WithContext(context_->getGlobalContext()), db_disk(context_->getDatabaseDisk()), log(getLogger(logger)) diff --git a/src/Databases/DatabasesCommon.h b/src/Databases/DatabasesCommon.h index c6b2c1ea770a..a788bf034079 100644 --- a/src/Databases/DatabasesCommon.h +++ b/src/Databases/DatabasesCommon.h @@ -23,6 +23,8 @@ void cleanupObjectDefinitionFromTemporaryFlags(ASTCreateQuery & query); String readMetadataFile(std::shared_ptr db_disk, const String & file_path); void writeMetadataFile(std::shared_ptr db_disk, const String & file_path, std::string_view content, bool fsync_metadata); +void updateDatabaseCommentWithMetadataFile(DatabasePtr db, const AlterCommand & command); + /// A base class for databases that manage their own list of tables. class DatabaseWithOwnTablesBase : public IDatabase, protected WithContext { diff --git a/src/Databases/IDatabase.cpp b/src/Databases/IDatabase.cpp index bbd8efe2df64..fbd418924867 100644 --- a/src/Databases/IDatabase.cpp +++ b/src/Databases/IDatabase.cpp @@ -58,6 +58,11 @@ IDatabase::~IDatabase() CurrentMetrics::sub(CurrentMetrics::AttachedDatabase, 1); } +void IDatabase::alterDatabaseComment(const AlterCommand & /*command*/) +{ + throw Exception(ErrorCodes::NOT_IMPLEMENTED, "{}: ALTER DATABASE COMMENT is not supported", getEngineName()); +} + std::vector> IDatabase::getTablesForBackup(const FilterByNameFunction &, const ContextPtr &) const { /// Cannot backup any table because IDatabase doesn't own any tables. diff --git a/src/Databases/IDatabase.h b/src/Databases/IDatabase.h index 28caebe6d1f1..c284b20d204c 100644 --- a/src/Databases/IDatabase.h +++ b/src/Databases/IDatabase.h @@ -21,12 +21,14 @@ namespace DB { + struct Settings; struct ConstraintsDescription; struct IndicesDescription; struct StorageInMemoryMetadata; struct StorageID; class ASTCreateQuery; +struct AlterCommand; class AlterCommands; class SettingsChanges; using DictionariesWithID = std::vector>; @@ -376,6 +378,9 @@ class IDatabase : public std::enable_shared_from_this return database_name; } + // Alter comment of database. + virtual void alterDatabaseComment(const AlterCommand &); + /// Get UUID of database. virtual UUID getUUID() const { return UUIDHelpers::Nil; } diff --git a/src/Databases/MySQL/DatabaseMySQL.cpp b/src/Databases/MySQL/DatabaseMySQL.cpp index 8137c2fda2a0..6cf0e0afd212 100644 --- a/src/Databases/MySQL/DatabaseMySQL.cpp +++ b/src/Databases/MySQL/DatabaseMySQL.cpp @@ -16,6 +16,7 @@ # include # include # include +# include # include # include # include @@ -25,6 +26,7 @@ # include # include # include +# include # include # include # include @@ -422,6 +424,11 @@ StoragePtr DatabaseMySQL::detachTable(ContextPtr /* context */, const String & t return local_tables_cache[table_name].second; } +void DatabaseMySQL::alterDatabaseComment(const AlterCommand & command) +{ + DB::updateDatabaseCommentWithMetadataFile(shared_from_this(), command); +} + String DatabaseMySQL::getMetadataPath() const { return metadata_path; diff --git a/src/Databases/MySQL/DatabaseMySQL.h b/src/Databases/MySQL/DatabaseMySQL.h index 71b117d4d463..f10a4f8f3483 100644 --- a/src/Databases/MySQL/DatabaseMySQL.h +++ b/src/Databases/MySQL/DatabaseMySQL.h @@ -23,6 +23,7 @@ namespace DB { class Context; +struct AlterCommand; struct MySQLSettings; enum class MySQLDataTypesSupport : uint8_t; @@ -83,6 +84,8 @@ class DatabaseMySQL final : public IDatabase, WithContext void attachTable(ContextPtr context, const String & table_name, const StoragePtr & storage, const String & relative_table_path) override; + void alterDatabaseComment(const AlterCommand & command) override; + protected: ASTPtr getCreateTableQueryImpl(const String & name, ContextPtr context, bool throw_on_error) const override; diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp index 9e698bdd6d20..c589c9841846 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp @@ -6,9 +6,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -395,6 +397,10 @@ void DatabasePostgreSQL::shutdown() cleaner_task->deactivate(); } +void DatabasePostgreSQL::alterDatabaseComment(const AlterCommand & command) +{ + DB::updateDatabaseCommentWithMetadataFile(shared_from_this(), command); +} ASTPtr DatabasePostgreSQL::getCreateDatabaseQuery() const { diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.h b/src/Databases/PostgreSQL/DatabasePostgreSQL.h index be32945aa994..b805c7fab687 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.h +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.h @@ -13,6 +13,7 @@ namespace DB { class Context; +struct AlterCommand; /** Real-time access to table list and table structure from remote PostgreSQL. @@ -60,6 +61,8 @@ class DatabasePostgreSQL final : public IDatabase, WithContext void drop(ContextPtr /*context*/) override; void shutdown() override; + void alterDatabaseComment(const AlterCommand & command) override; + protected: ASTPtr getCreateTableQueryImpl(const String & table_name, ContextPtr context, bool throw_on_error) const override; diff --git a/src/Databases/SQLite/DatabaseSQLite.cpp b/src/Databases/SQLite/DatabaseSQLite.cpp index fb01baae7c2d..d004f64fc5ff 100644 --- a/src/Databases/SQLite/DatabaseSQLite.cpp +++ b/src/Databases/SQLite/DatabaseSQLite.cpp @@ -2,12 +2,14 @@ #if USE_SQLITE +#include #include #include #include #include #include #include +#include #include #include #include @@ -178,6 +180,10 @@ ASTPtr DatabaseSQLite::getCreateDatabaseQuery() const return create_query; } +void DatabaseSQLite::alterDatabaseComment(const AlterCommand & command) +{ + DB::updateDatabaseCommentWithMetadataFile(shared_from_this(), command); +} ASTPtr DatabaseSQLite::getCreateTableQueryImpl(const String & table_name, ContextPtr local_context, bool throw_on_error) const { diff --git a/src/Databases/SQLite/DatabaseSQLite.h b/src/Databases/SQLite/DatabaseSQLite.h index 6bd84a4d297b..273e178313a6 100644 --- a/src/Databases/SQLite/DatabaseSQLite.h +++ b/src/Databases/SQLite/DatabaseSQLite.h @@ -12,6 +12,8 @@ namespace DB { +struct AlterCommand; + class DatabaseSQLite final : public IDatabase, WithContext { public: @@ -40,6 +42,8 @@ class DatabaseSQLite final : public IDatabase, WithContext void shutdown() override {} + void alterDatabaseComment(const AlterCommand & command) override; + protected: ASTPtr getCreateTableQueryImpl(const String & table_name, ContextPtr context, bool throw_on_error) const override; diff --git a/src/Interpreters/DatabaseCatalog.cpp b/src/Interpreters/DatabaseCatalog.cpp index 2d603590cd96..06f8d4716578 100644 --- a/src/Interpreters/DatabaseCatalog.cpp +++ b/src/Interpreters/DatabaseCatalog.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -74,6 +75,12 @@ namespace ErrorCodes extern const int HAVE_DEPENDENT_OBJECTS; extern const int UNFINISHED; extern const int INFINITE_LOOP; + extern const int THERE_IS_NO_QUERY; +} + +namespace Setting +{ + extern const SettingsBool fsync_metadata; } class DatabaseNameHints : public IHints<> @@ -659,6 +666,42 @@ void DatabaseCatalog::updateDatabaseName(const String & old_name, const String & } } +void DatabaseCatalog::updateMetadataFile(const DatabasePtr & database) +{ + std::lock_guard lock{databases_mutex}; + ASTPtr ast = database->getCreateDatabaseQuery(); + if (!ast) + throw Exception( + ErrorCodes::THERE_IS_NO_QUERY, "Unable to show the create query of database {}", backQuoteIfNeed(database->getDatabaseName())); + + auto * ast_create_query = ast->as(); + ast_create_query->attach = true; + + WriteBufferFromOwnString statement_buf; + IAST::FormatSettings format_settings(/*one_line=*/false, /*hilite*/false); + ast_create_query->format(statement_buf, format_settings); + writeChar('\n', statement_buf); + String statement = statement_buf.str(); + + auto database_metadata_tmp_path = fs::path("metadata") / (escapeForFileName(database->getDatabaseName()) + ".sql.tmp"); + auto database_metadata_path = fs::path("metadata") / (escapeForFileName(database->getDatabaseName()) + ".sql"); + auto db_disk = getContext()->getDatabaseDisk(); + + writeMetadataFile( + db_disk, /*file_path=*/database_metadata_tmp_path, /*content=*/statement, getContext()->getSettingsRef()[Setting::fsync_metadata]); + + try + { + /// rename atomically replaces the old file with the new one. + db_disk->replaceFile(database_metadata_tmp_path, database_metadata_path); + } + catch (...) + { + db_disk->removeFileIfExists(database_metadata_tmp_path); + throw; + } +} + DatabasePtr DatabaseCatalog::getDatabase(const String & database_name) const { assert(!database_name.empty()); diff --git a/src/Interpreters/DatabaseCatalog.h b/src/Interpreters/DatabaseCatalog.h index 55c4836cfd25..ec9acbe71189 100644 --- a/src/Interpreters/DatabaseCatalog.h +++ b/src/Interpreters/DatabaseCatalog.h @@ -271,6 +271,8 @@ class DatabaseCatalog : boost::noncopyable, WithMutableContext void startReplicatedDDLQueries(); bool canPerformReplicatedDDLQueries() const; + void updateMetadataFile(const DatabasePtr & database); + private: // The global instance of database catalog. unique_ptr is to allow // deferred initialization. Thought I'd use std::optional, but I can't diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index 17793cf8f7e6..d527bdabac31 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -38,6 +38,7 @@ namespace DB namespace Setting { extern const SettingsBool allow_experimental_statistics; + extern const SettingsBool fsync_metadata; extern const SettingsSeconds lock_acquire_timeout; } @@ -58,7 +59,6 @@ namespace ErrorCodes extern const int QUERY_IS_PROHIBITED; } - InterpreterAlterQuery::InterpreterAlterQuery(const ASTPtr & query_ptr_, ContextPtr context_) : WithContext(context_), query_ptr(query_ptr_) { } @@ -266,20 +266,27 @@ BlockIO InterpreterAlterQuery::executeToDatabase(const ASTAlterQuery & alter) if (!alter_commands.empty()) { - /// Only ALTER SETTING is supported. + /// Only ALTER SETTING and ALTER COMMENT is supported. for (const auto & command : alter_commands) { - if (command.type != AlterCommand::MODIFY_DATABASE_SETTING) + if (command.type != AlterCommand::MODIFY_DATABASE_SETTING && command.type != AlterCommand::MODIFY_DATABASE_COMMENT) throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Unsupported alter type for database engines"); } for (const auto & command : alter_commands) { - if (!command.ignore) + if (command.ignore) + continue; + + switch (command.type) { - if (command.type == AlterCommand::MODIFY_DATABASE_SETTING) + case AlterCommand::MODIFY_DATABASE_SETTING: database->applySettingsChanges(command.settings_changes, getContext()); - else + break; + case AlterCommand::MODIFY_DATABASE_COMMENT: + database->alterDatabaseComment(command); + break; + default: throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Unsupported alter command"); } } @@ -520,6 +527,11 @@ AccessRightsElements InterpreterAlterQuery::getRequiredAccessForCommand(const AS required_access.emplace_back(AccessType::ALTER_DATABASE_SETTINGS, database, table); break; } + case ASTAlterCommand::MODIFY_DATABASE_COMMENT: + { + required_access.emplace_back(AccessType::ALTER_MODIFY_DATABASE_COMMENT, database, table); + break; + } case ASTAlterCommand::NO_TYPE: break; case ASTAlterCommand::MODIFY_COMMENT: { diff --git a/src/Parsers/ASTAlterQuery.cpp b/src/Parsers/ASTAlterQuery.cpp index af0ec89a259e..a9d395d358da 100644 --- a/src/Parsers/ASTAlterQuery.cpp +++ b/src/Parsers/ASTAlterQuery.cpp @@ -161,7 +161,7 @@ void ASTAlterCommand::formatImpl(WriteBuffer & ostr, const FormatSettings & sett ostr << " " << (settings.hilite ? hilite_none : ""); comment->format(ostr, settings, state, frame); } - else if (type == ASTAlterCommand::MODIFY_COMMENT) + else if (type == ASTAlterCommand::MODIFY_COMMENT || type == ASTAlterCommand::MODIFY_DATABASE_COMMENT) { ostr << (settings.hilite ? hilite_keyword : "") << "MODIFY COMMENT" << (settings.hilite ? hilite_none : ""); ostr << " " << (settings.hilite ? hilite_none : ""); diff --git a/src/Parsers/ASTAlterQuery.h b/src/Parsers/ASTAlterQuery.h index 347735a24645..b9db42f17878 100644 --- a/src/Parsers/ASTAlterQuery.h +++ b/src/Parsers/ASTAlterQuery.h @@ -79,8 +79,10 @@ class ASTAlterCommand : public IAST NO_TYPE, MODIFY_DATABASE_SETTING, + MODIFY_DATABASE_COMMENT, MODIFY_COMMENT, + MODIFY_SQL_SECURITY, UNLOCK_SNAPSHOT, diff --git a/src/Parsers/CommonParsers.h b/src/Parsers/CommonParsers.h index 62b5ca135cd8..8d69ae5d5fe5 100644 --- a/src/Parsers/CommonParsers.h +++ b/src/Parsers/CommonParsers.h @@ -314,6 +314,7 @@ namespace DB MR_MACROS(MOD, "MOD") \ MR_MACROS(MODIFY_COLUMN, "MODIFY COLUMN") \ MR_MACROS(MODIFY_COMMENT, "MODIFY COMMENT") \ + MR_MACROS(MODIFY_DATABASE_COMMENT, "MODIFY DATABASE COMMENT") \ MR_MACROS(MODIFY_DEFINER, "MODIFY DEFINER") \ MR_MACROS(MODIFY_ORDER_BY, "MODIFY ORDER BY") \ MR_MACROS(MODIFY_QUERY, "MODIFY QUERY") \ diff --git a/src/Parsers/ParserAlterQuery.cpp b/src/Parsers/ParserAlterQuery.cpp index 8590fbac80d8..b543992bf617 100644 --- a/src/Parsers/ParserAlterQuery.cpp +++ b/src/Parsers/ParserAlterQuery.cpp @@ -190,6 +190,13 @@ bool ParserAlterCommand::parseImpl(Pos & pos, ASTPtr & node, Expected & expected return false; command->type = ASTAlterCommand::MODIFY_DATABASE_SETTING; } + else if (s_modify_comment.ignore(pos, expected)) + { + if (!parser_string_literal.parse(pos, command_comment, expected)) + return false; + + command->type = ASTAlterCommand::MODIFY_DATABASE_COMMENT; + } else return false; break; diff --git a/src/Storages/AlterCommands.cpp b/src/Storages/AlterCommands.cpp index 4640c49ff70c..57bbd9edb11b 100644 --- a/src/Storages/AlterCommands.cpp +++ b/src/Storages/AlterCommands.cpp @@ -239,6 +239,15 @@ std::optional AlterCommand::parse(const ASTAlterCommand * command_ command.comment = ast_comment.value.safeGet(); return command; } + if (command_ast->type == ASTAlterCommand::MODIFY_DATABASE_COMMENT) + { + AlterCommand command; + command.ast = command_ast->clone(); + command.type = MODIFY_DATABASE_COMMENT; + const auto & ast_comment = command_ast->comment->as(); + command.comment = ast_comment.value.safeGet(); + return command; + } if (command_ast->type == ASTAlterCommand::MODIFY_ORDER_BY) { AlterCommand command; diff --git a/src/Storages/AlterCommands.h b/src/Storages/AlterCommands.h index be1b31f3d20c..ded5b182df10 100644 --- a/src/Storages/AlterCommands.h +++ b/src/Storages/AlterCommands.h @@ -49,6 +49,7 @@ struct AlterCommand RENAME_COLUMN, REMOVE_TTL, MODIFY_DATABASE_SETTING, + MODIFY_DATABASE_COMMENT, COMMENT_TABLE, REMOVE_SAMPLE_BY, MODIFY_SQL_SECURITY, diff --git a/tests/integration/test_mysql_database_engine/test.py b/tests/integration/test_mysql_database_engine/test.py index 7ce805b330a1..a689ebc6ef89 100644 --- a/tests/integration/test_mysql_database_engine/test.py +++ b/tests/integration/test_mysql_database_engine/test.py @@ -1044,3 +1044,39 @@ def test_password_leak(started_cluster): assert "clickhouse" not in clickhouse_node.query( "SHOW CREATE test_database.test_table" ) + + +def test_mysql_database_engine_comment(started_cluster): + with contextlib.closing( + MySQLNodeInstance( + "root", mysql_pass, started_cluster.mysql8_ip, started_cluster.mysql8_port + ) + ) as mysql_node: + mysql_node.query("DROP DATABASE IF EXISTS test_database") + mysql_node.query("CREATE DATABASE test_database DEFAULT CHARACTER SET 'utf8'") + + clickhouse_node.query("DROP DATABASE IF EXISTS test_database") + clickhouse_node.query( + f"CREATE DATABASE test_database ENGINE = MySQL('mysql80:3306', 'test_database', 'root', '{mysql_pass}') \ + comment 'test mysql database engine comment'" + ) + assert "test_database" in clickhouse_node.query("SHOW DATABASES") + + assert ( + clickhouse_node.query("SELECT comment FROM system.databases WHERE name='test_database'").rstrip() + == "test mysql database engine comment" + ) + + clickhouse_node.query( + "ALTER DATABASE test_database MODIFY COMMENT 'new comment on mysql database engine'" + ) + + assert ( + clickhouse_node.query("SELECT comment FROM system.databases WHERE name='test_database'").rstrip() + == "new comment on mysql database engine" + ) + + clickhouse_node.query("DROP DATABASE test_database") + assert "test_database" not in clickhouse_node.query("SHOW DATABASES") + + mysql_node.query("DROP DATABASE test_database") \ No newline at end of file diff --git a/tests/integration/test_postgresql_database_engine/test.py b/tests/integration/test_postgresql_database_engine/test.py index dba4c36859a4..d9a3100359c2 100644 --- a/tests/integration/test_postgresql_database_engine/test.py +++ b/tests/integration/test_postgresql_database_engine/test.py @@ -465,6 +465,29 @@ def test_inaccessible_postgresql_database_engine_filterable_on_system_tables( node1.query("DROP DATABASE postgres_database") assert "postgres_database" not in node1.query("SHOW DATABASES") +def test_postgresql_database_engine_comment(started_cluster): + conn = get_postgres_conn( + started_cluster.postgres_ip, started_cluster.postgres_port, database=True + ) + cursor = conn.cursor() + + node1.query( + "CREATE DATABASE postgres_database ENGINE = PostgreSQL('postgres1:5432', 'postgres_database', 'postgres', 'mysecretpassword') \ + comment 'test postgres database with comment'" + ) + + node1.query( + "ALTER DATABASE postgres_database MODIFY COMMENT 'new comment on postgres database engine'" + ) + + assert ( + node1.query("SELECT comment FROM system.databases WHERE name = 'postgres_database'").rstrip() + == "new comment on postgres database engine" + ) + + node1.query("DROP DATABASE postgres_database") + assert "postgres_database" not in node1.query("SHOW DATABASES") + if __name__ == "__main__": cluster.start() diff --git a/tests/queries/0_stateless/01271_show_privileges.reference b/tests/queries/0_stateless/01271_show_privileges.reference index e5107526be91..0ac4f68abc21 100644 --- a/tests/queries/0_stateless/01271_show_privileges.reference +++ b/tests/queries/0_stateless/01271_show_privileges.reference @@ -18,6 +18,7 @@ ALTER RENAME COLUMN ['RENAME COLUMN'] COLUMN ALTER COLUMN ALTER MATERIALIZE COLUMN ['MATERIALIZE COLUMN'] COLUMN ALTER COLUMN ALTER COLUMN [] \N ALTER TABLE ALTER MODIFY COMMENT ['MODIFY COMMENT'] TABLE ALTER TABLE +ALTER MODIFY DATABASE COMMENT ['MODIFY DATABASE COMMENT'] DATABASE ALTER DATABASE ALTER ORDER BY ['ALTER MODIFY ORDER BY','MODIFY ORDER BY'] TABLE ALTER INDEX ALTER SAMPLE BY ['ALTER MODIFY SAMPLE BY','MODIFY SAMPLE BY'] TABLE ALTER INDEX ALTER ADD INDEX ['ADD INDEX'] TABLE ALTER INDEX diff --git a/tests/queries/0_stateless/03328_alter_database_modify_comment.reference b/tests/queries/0_stateless/03328_alter_database_modify_comment.reference new file mode 100644 index 000000000000..764b307cf0a6 --- /dev/null +++ b/tests/queries/0_stateless/03328_alter_database_modify_comment.reference @@ -0,0 +1,52 @@ +databasename test_database_03328_alter_database_modify_comment_default +engine : Atomic +initial comment +name= test_database_03328_alter_database_modify_comment_default comment= Test database with comment + +change a comment +name= test_database_03328_alter_database_modify_comment_default comment= new comment on database + +add a comment back +name= test_database_03328_alter_database_modify_comment_default comment= another comment on database + +detach database + +re-attach database +name= test_database_03328_alter_database_modify_comment_default comment= another comment on database + +drop database + +engine : Lazy +initial comment +name= test_database_03328_alter_database_modify_comment_default comment= Test database with comment + +change a comment +name= test_database_03328_alter_database_modify_comment_default comment= new comment on database + +add a comment back +name= test_database_03328_alter_database_modify_comment_default comment= another comment on database + +detach database + +re-attach database +name= test_database_03328_alter_database_modify_comment_default comment= another comment on database + +drop database + +engine : Memory +initial comment +name= test_database_03328_alter_database_modify_comment_default comment= Test database with comment + +change a comment +name= test_database_03328_alter_database_modify_comment_default comment= new comment on database + +add a comment back +name= test_database_03328_alter_database_modify_comment_default comment= another comment on database + +detach database + +re-attach database +name= test_database_03328_alter_database_modify_comment_default comment= another comment on database + +drop database + diff --git a/tests/queries/0_stateless/03328_alter_database_modify_comment.sh b/tests/queries/0_stateless/03328_alter_database_modify_comment.sh new file mode 100755 index 000000000000..63674b70a4c8 --- /dev/null +++ b/tests/queries/0_stateless/03328_alter_database_modify_comment.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env bash + +CUR_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CUR_DIR"/../shell_config.sh + +databasename="test_database_${CLICKHOUSE_TEST_UNIQUE_NAME}" + +function get_database_comment_info() +{ + $CLICKHOUSE_CLIENT -mq "SELECT 'name=', name, 'comment=', comment \ + FROM system.databases where name='${databasename}'" + echo # just a newline +} + +echo "databasename " ${databasename} + +function test_database_comments() +{ + local ENGINE_NAME="$1" + echo "engine : ${ENGINE_NAME}" + + $CLICKHOUSE_CLIENT -mq "DROP DATABASE IF EXISTS ${databasename}"; + + if [ "$ENGINE_NAME" = "Atomic" ]; then + $CLICKHOUSE_CLIENT -mq "CREATE DATABASE ${databasename} ENGINE = Atomic COMMENT 'Test database with comment';" + elif [ "$ENGINE_NAME" = "Lazy" ]; then + $CLICKHOUSE_CLIENT -mq "CREATE DATABASE ${databasename} ENGINE = Lazy(1) COMMENT 'Test database with comment';" + elif [ "$ENGINE_NAME" = "Memory" ]; then + $CLICKHOUSE_CLIENT -mq "CREATE DATABASE ${databasename} ENGINE = Memory COMMENT 'Test database with comment';" + else + echo "Unknown ENGINE_NAME: $ENGINE_NAME" + fi + + echo initial comment + get_database_comment_info + + echo change a comment + $CLICKHOUSE_CLIENT -mq "ALTER DATABASE ${databasename} MODIFY COMMENT 'new comment on database';" + get_database_comment_info + + echo add a comment back + $CLICKHOUSE_CLIENT -mq "ALTER DATABASE ${databasename} MODIFY COMMENT 'another comment on database';" + get_database_comment_info + + echo detach database + $CLICKHOUSE_CLIENT -mq "DETACH DATABASE ${databasename} SYNC;" + get_database_comment_info + + echo re-attach database + $CLICKHOUSE_CLIENT -mq "ATTACH DATABASE ${databasename};" + get_database_comment_info + + echo drop database + $CLICKHOUSE_CLIENT -mq "DROP DATABASE ${databasename};" + get_database_comment_info +} + +test_database_comments "Atomic" +test_database_comments "Lazy" +test_database_comments "Memory" \ No newline at end of file diff --git a/tests/queries/0_stateless/03329_grant_alter_database_comment.reference b/tests/queries/0_stateless/03329_grant_alter_database_comment.reference new file mode 100644 index 000000000000..b41a34851e52 --- /dev/null +++ b/tests/queries/0_stateless/03329_grant_alter_database_comment.reference @@ -0,0 +1,4 @@ +GRANT ALTER MODIFY DATABASE COMMENT ON test_database_03329_grant_alter_database_comment_default.* TO test_user_03329_grant_alter_database_comment_default +GRANT ALTER MODIFY DATABASE COMMENT ON test_database_03329_grant_alter_database_comment_default.* +test_database_03329_grant_alter_database_comment_default test database with comment +test_database_03329_grant_alter_database_comment_default new comment on database diff --git a/tests/queries/0_stateless/03329_grant_alter_database_comment.sh b/tests/queries/0_stateless/03329_grant_alter_database_comment.sh new file mode 100755 index 000000000000..4ae016422d48 --- /dev/null +++ b/tests/queries/0_stateless/03329_grant_alter_database_comment.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=../shell_config.sh +. "$CURDIR"/../shell_config.sh + +databasename="test_database_${CLICKHOUSE_TEST_UNIQUE_NAME}" +username="test_user_${CLICKHOUSE_TEST_UNIQUE_NAME}" + +${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS ${username}"; +${CLICKHOUSE_CLIENT} --query "DROP DATABASE IF EXISTS ${databasename}"; + +${CLICKHOUSE_CLIENT} --query "CREATE USER ${username};"; +${CLICKHOUSE_CLIENT} --query "CREATE DATABASE ${databasename} COMMENT 'test database with comment';"; + +${CLICKHOUSE_CLIENT} --query "GRANT ALTER MODIFY DATABASE COMMENT ON ${databasename}.* TO ${username};"; + +${CLICKHOUSE_CLIENT} --query "SHOW GRANTS FOR ${username};"; + +${CLICKHOUSE_CLIENT} --user="${username}" --query "SHOW GRANTS FOR ${username}" | sed 's/ TO.*//'; + +${CLICKHOUSE_CLIENT} --user="${username}" --query "SELECT name, comment FROM system.databases WHERE name = '${databasename}';"; + +${CLICKHOUSE_CLIENT} --user="${username}" --query "ALTER DATABASE ${databasename} MODIFY COMMENT 'new comment on database';" + +${CLICKHOUSE_CLIENT} --user="${username}" --query "SELECT name, comment FROM system.databases WHERE name = '${databasename}';"; + +${CLICKHOUSE_CLIENT} --query "REVOKE ALTER MODIFY DATABASE COMMENT ON ${databasename}.* FROM ${username};"; + +${CLICKHOUSE_CLIENT} --user="${username}" --query "SHOW GRANTS FOR ${username};"; + +${CLICKHOUSE_CLIENT} --user="${username}" --query "ALTER DATABASE ${databasename} MODIFY COMMENT 'test alter comment after revoking;' \ + -- { serverError ACCESS_DENIED } "; + +${CLICKHOUSE_CLIENT} --query "DROP USER IF EXISTS ${username}"; + +${CLICKHOUSE_CLIENT} --query "DROP DATABASE IF EXISTS ${databasename}"; \ No newline at end of file