From 2f368625c55b054e66f1d967b96935d8f3cd64b1 Mon Sep 17 00:00:00 2001 From: TCeason Date: Tue, 10 Sep 2024 10:40:13 +0800 Subject: [PATCH] use mget_databases replace for --- src/query/catalog/src/catalog/interface.rs | 8 ++ .../src/catalogs/default/database_catalog.rs | 33 +++++++ .../src/catalogs/default/immutable_catalog.rs | 18 ++++ .../src/catalogs/default/mutable_catalog.rs | 29 +++++++ .../src/catalogs/default/session_catalog.rs | 9 ++ .../tests/it/sql/exec/get_table_bind_test.rs | 9 ++ .../it/storages/fuse/operations/commit.rs | 9 ++ .../storages/hive/hive/src/hive_catalog.rs | 11 +++ src/query/storages/iceberg/src/catalog.rs | 10 +++ .../storages/system/src/columns_table.rs | 50 +++++------ .../storages/system/src/databases_table.rs | 86 ++++++++----------- src/query/storages/system/src/tables_table.rs | 54 ++++++++++-- .../18_rbac/18_0013_column_privilege.result | 3 + .../18_rbac/18_0013_column_privilege.sh | 8 +- 14 files changed, 252 insertions(+), 85 deletions(-) diff --git a/src/query/catalog/src/catalog/interface.rs b/src/query/catalog/src/catalog/interface.rs index 3dd5c050a6a0..ac4debc04eb8 100644 --- a/src/query/catalog/src/catalog/interface.rs +++ b/src/query/catalog/src/catalog/interface.rs @@ -19,6 +19,7 @@ use std::sync::Arc; use databend_common_config::InnerConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CommitTableMetaReply; @@ -223,6 +224,13 @@ pub trait Catalog: DynClone + Send + Sync + Debug { // Get the db name by meta id. async fn get_db_name_by_id(&self, db_ids: MetaId) -> Result; + // Mget dbs by DatabaseNameIdent. + async fn mget_databases( + &self, + tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>>; + // Mget the dbs name by meta ids. async fn mget_database_names_by_ids( &self, diff --git a/src/query/service/src/catalogs/default/database_catalog.rs b/src/query/service/src/catalogs/default/database_catalog.rs index c0f351168181..5cb096fb11dc 100644 --- a/src/query/service/src/catalogs/default/database_catalog.rs +++ b/src/query/service/src/catalogs/default/database_catalog.rs @@ -25,6 +25,7 @@ use databend_common_catalog::table_function::TableFunction; use databend_common_config::InnerConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CommitTableMetaReply; @@ -338,6 +339,38 @@ impl Catalog for DatabaseCatalog { } } + async fn mget_databases( + &self, + tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>> { + let sys_dbs = self.immutable_catalog.list_databases(tenant).await?; + let sys_db_names: Vec<_> = sys_dbs + .iter() + .map(|sys_db| sys_db.get_db_info().name_ident.database_name()) + .collect(); + + let mut mut_db_names: Vec<_> = Vec::new(); + for db_name in db_names { + if !sys_db_names.contains(&db_name.database_name()) { + mut_db_names.push(db_name.clone()); + } + } + + let mut dbs = self + .immutable_catalog + .mget_databases(tenant, db_names) + .await?; + + let other = self + .mutable_catalog + .mget_databases(tenant, &mut_db_names) + .await?; + + dbs.extend(other); + Ok(dbs) + } + #[async_backtrace::framed] async fn mget_database_names_by_ids( &self, diff --git a/src/query/service/src/catalogs/default/immutable_catalog.rs b/src/query/service/src/catalogs/default/immutable_catalog.rs index f2800928756e..ae49d5f3690a 100644 --- a/src/query/service/src/catalogs/default/immutable_catalog.rs +++ b/src/query/service/src/catalogs/default/immutable_catalog.rs @@ -21,6 +21,7 @@ use databend_common_catalog::catalog::Catalog; use databend_common_config::InnerConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CommitTableMetaReply; @@ -232,6 +233,23 @@ impl Catalog for ImmutableCatalog { } } + async fn mget_databases( + &self, + _tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>> { + let mut res: Vec> = vec![]; + for db_name in db_names { + let db_name = db_name.database_name(); + if db_name == "system" { + res.push(self.sys_db.clone()); + } else if db_name == "information_schema" { + res.push(self.info_schema_db.clone()); + } + } + Ok(res) + } + async fn mget_database_names_by_ids( &self, _tenant: &Tenant, diff --git a/src/query/service/src/catalogs/default/mutable_catalog.rs b/src/query/service/src/catalogs/default/mutable_catalog.rs index c7e3ad3de1f6..d1adbe2137c1 100644 --- a/src/query/service/src/catalogs/default/mutable_catalog.rs +++ b/src/query/service/src/catalogs/default/mutable_catalog.rs @@ -22,6 +22,7 @@ use databend_common_catalog::catalog::Catalog; use databend_common_config::InnerConfig; use databend_common_exception::Result; use databend_common_meta_api::kv_app_error::KVAppError; +use databend_common_meta_api::name_id_value_api::NameIdValueApiCompat; use databend_common_meta_api::SchemaApi; use databend_common_meta_api::SequenceApi; use databend_common_meta_app::app_error::AppError; @@ -422,6 +423,34 @@ impl Catalog for MutableCatalog { Ok(res) } + // Mget dbs by DatabaseNameIdent. + async fn mget_databases( + &self, + _tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>> { + let res = self + .ctx + .meta + .mget_id_value_compat(db_names.iter().cloned()) + .await?; + let dbs = res + .map(|(name_ident, database_id, meta)| { + Arc::new(DatabaseInfo { + database_id, + name_ident, + meta, + }) + }) + .collect::>>(); + + dbs.iter().try_fold(vec![], |mut acc, item| { + let db = self.build_db_instance(item)?; + acc.push(db); + Ok(acc) + }) + } + async fn mget_database_names_by_ids( &self, _tenant: &Tenant, diff --git a/src/query/service/src/catalogs/default/session_catalog.rs b/src/query/service/src/catalogs/default/session_catalog.rs index e57b9d109f33..46f10275e701 100644 --- a/src/query/service/src/catalogs/default/session_catalog.rs +++ b/src/query/service/src/catalogs/default/session_catalog.rs @@ -23,6 +23,7 @@ use databend_common_catalog::table_args::TableArgs; use databend_common_catalog::table_function::TableFunction; use databend_common_exception::ErrorCode; use databend_common_exception::Result; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CommitTableMetaReply; @@ -305,6 +306,14 @@ impl Catalog for SessionCatalog { self.inner.get_db_name_by_id(db_id).await } + async fn mget_databases( + &self, + tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>> { + self.inner.mget_databases(tenant, db_names).await + } + // Mget the dbs name by meta ids. async fn mget_database_names_by_ids( &self, diff --git a/src/query/service/tests/it/sql/exec/get_table_bind_test.rs b/src/query/service/tests/it/sql/exec/get_table_bind_test.rs index 873abc01fdf1..cd6f3cd9e7a4 100644 --- a/src/query/service/tests/it/sql/exec/get_table_bind_test.rs +++ b/src/query/service/tests/it/sql/exec/get_table_bind_test.rs @@ -55,6 +55,7 @@ use databend_common_meta_app::principal::RoleInfo; use databend_common_meta_app::principal::UserDefinedConnection; use databend_common_meta_app::principal::UserInfo; use databend_common_meta_app::principal::UserPrivilegeType; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CommitTableMetaReply; @@ -208,6 +209,14 @@ impl Catalog for FakedCatalog { self.cat.get_db_name_by_id(db_id).await } + async fn mget_databases( + &self, + tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>> { + self.cat.mget_databases(tenant, db_names).await + } + async fn mget_database_names_by_ids( &self, tenant: &Tenant, diff --git a/src/query/service/tests/it/storages/fuse/operations/commit.rs b/src/query/service/tests/it/storages/fuse/operations/commit.rs index 735f0963741f..7acaf862daa2 100644 --- a/src/query/service/tests/it/storages/fuse/operations/commit.rs +++ b/src/query/service/tests/it/storages/fuse/operations/commit.rs @@ -54,6 +54,7 @@ use databend_common_meta_app::principal::RoleInfo; use databend_common_meta_app::principal::UserDefinedConnection; use databend_common_meta_app::principal::UserInfo; use databend_common_meta_app::principal::UserPrivilegeType; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CommitTableMetaReply; @@ -957,6 +958,14 @@ impl Catalog for FakedCatalog { self.cat.get_db_name_by_id(db_id).await } + async fn mget_databases( + &self, + tenant: &Tenant, + db_names: &[DatabaseNameIdent], + ) -> Result>> { + self.cat.mget_databases(tenant, db_names).await + } + #[async_backtrace::framed] async fn mget_database_names_by_ids( &self, diff --git a/src/query/storages/hive/hive/src/hive_catalog.rs b/src/query/storages/hive/hive/src/hive_catalog.rs index 944a03e8871a..d50db4340e97 100644 --- a/src/query/storages/hive/hive/src/hive_catalog.rs +++ b/src/query/storages/hive/hive/src/hive_catalog.rs @@ -28,6 +28,7 @@ use databend_common_catalog::table_function::TableFunction; use databend_common_config::InnerConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CatalogOption; @@ -385,6 +386,16 @@ impl Catalog for HiveCatalog { )) } + async fn mget_databases( + &self, + _tenant: &Tenant, + _db_names: &[DatabaseNameIdent], + ) -> Result>> { + Err(ErrorCode::Unimplemented( + "Cannot mget databases in HIVE catalog", + )) + } + async fn mget_database_names_by_ids( &self, _tenant: &Tenant, diff --git a/src/query/storages/iceberg/src/catalog.rs b/src/query/storages/iceberg/src/catalog.rs index 90ed0b3ac17e..837f64432a7f 100644 --- a/src/query/storages/iceberg/src/catalog.rs +++ b/src/query/storages/iceberg/src/catalog.rs @@ -26,6 +26,7 @@ use databend_common_catalog::table_function::TableFunction; use databend_common_config::InnerConfig; use databend_common_exception::ErrorCode; use databend_common_exception::Result; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::dictionary_name_ident::DictionaryNameIdent; use databend_common_meta_app::schema::CatalogInfo; use databend_common_meta_app::schema::CatalogOption; @@ -295,6 +296,15 @@ impl Catalog for IcebergCatalog { "Cannot get db name by id in ICEBERG catalog", )) } + async fn mget_databases( + &self, + _tenant: &Tenant, + _db_names: &[DatabaseNameIdent], + ) -> Result>> { + Err(ErrorCode::Unimplemented( + "Cannot mget databases in ICEBERG catalog", + )) + } async fn mget_database_names_by_ids( &self, diff --git a/src/query/storages/system/src/columns_table.rs b/src/query/storages/system/src/columns_table.rs index ab2da4d1453a..cecd2c0518df 100644 --- a/src/query/storages/system/src/columns_table.rs +++ b/src/query/storages/system/src/columns_table.rs @@ -28,6 +28,7 @@ use databend_common_expression::TableDataType; use databend_common_expression::TableField; use databend_common_expression::TableSchemaRefExt; use databend_common_functions::BUILTIN_FUNCTIONS; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::TableIdent; use databend_common_meta_app::schema::TableInfo; use databend_common_meta_app::schema::TableMeta; @@ -291,39 +292,38 @@ pub(crate) async fn dump_tables( } } else { let catalog_dbs = visibility_checker.get_visibility_database(); + // None means has global level privileges if let Some(catalog_dbs) = catalog_dbs { for (catalog_name, dbs) in catalog_dbs { if catalog_name == CATALOG_DEFAULT { let mut catalog_db_ids = vec![]; - for (db_name, db_id) in dbs { - if let Some(db_name) = db_name { - let db_id = catalog - .get_database(&tenant, db_name) - .await? - .get_db_info() - .database_id - .db_id; - final_dbs.push((db_name.to_string(), db_id)); - } - if let Some(db_id) = db_id { - catalog_db_ids.push(*db_id); - } - } - match catalog + let mut catalog_db_names = vec![]; + catalog_db_names.extend( + dbs.iter() + .filter_map(|(db_name, _)| *db_name) + .map(|db_name| db_name.to_string()), + ); + catalog_db_ids.extend(dbs.iter().filter_map(|(_, db_id)| *db_id)); + if let Ok(databases) = catalog .mget_database_names_by_ids(&tenant, &catalog_db_ids) .await { - Ok(databases) => { - for (i, db) in databases.into_iter().flatten().enumerate() { - final_dbs.push((db.to_string(), catalog_db_ids[i])); - } - } - Err(err) => { - let msg = - format!("Failed to get database: {}, {}", catalog.name(), err); - warn!("{}", msg); - } + catalog_db_names.extend(databases.into_iter().flatten()); + } else { + let msg = format!("Failed to get database name by id: {}", catalog.name()); + warn!("{}", msg); } + let db_idents = catalog_db_names + .iter() + .map(|name| DatabaseNameIdent::new(&tenant, name)) + .collect::>(); + let dbs: Vec<(String, u64)> = catalog + .mget_databases(&tenant, &db_idents) + .await? + .iter() + .map(|db| (db.name().to_string(), db.get_db_info().database_id.db_id)) + .collect(); + final_dbs.extend(dbs); } } } else { diff --git a/src/query/storages/system/src/databases_table.rs b/src/query/storages/system/src/databases_table.rs index 2951d72e8579..cfebe2c1b97f 100644 --- a/src/query/storages/system/src/databases_table.rs +++ b/src/query/storages/system/src/databases_table.rs @@ -29,6 +29,7 @@ use databend_common_expression::TableDataType; use databend_common_expression::TableField; use databend_common_expression::TableSchemaRefExt; use databend_common_meta_app::principal::OwnershipObject; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::TableIdent; use databend_common_meta_app::schema::TableInfo; use databend_common_meta_app::schema::TableMeta; @@ -74,62 +75,49 @@ impl AsyncSystemTable for DatabasesTable { let visibility_checker = ctx.get_visibility_checker().await?; let catalog_dbs = visibility_checker.get_visibility_database(); + // None means has global level privileges if let Some(catalog_dbs) = catalog_dbs { for (catalog, dbs) in catalog_dbs { let mut catalog_db_ids = vec![]; + let mut catalog_db_names = vec![]; let ctl = ctx.get_catalog(catalog).await?; - for (db_name, db_id) in dbs { - if let Some(db_name) = db_name { - let db_id = ctl - .get_database(&tenant, db_name) - .await? - .get_db_info() - .database_id - .db_id; - - catalog_names.push(catalog.clone()); - db_names.push(db_name.to_string()); - db_ids.push(db_id); - owners.push( - user_api - .get_ownership(&tenant, &OwnershipObject::Database { - catalog_name: catalog.clone(), - db_id, - }) - .await - .ok() - .and_then(|ownership| ownership.map(|o| o.role.clone())), - ); - } - if let Some(db_id) = db_id { - catalog_db_ids.push(*db_id); - } - } - match ctl + catalog_db_names.extend( + dbs.iter() + .filter_map(|(db_name, _)| *db_name) + .map(|db_name| db_name.to_string()), + ); + catalog_db_ids.extend(dbs.iter().filter_map(|(_, db_id)| *db_id)); + + if let Ok(databases) = ctl .mget_database_names_by_ids(&tenant, &catalog_db_ids) .await { - Ok(databases) => { - for (i, db) in databases.into_iter().flatten().enumerate() { - catalog_names.push(catalog.clone()); - db_names.push(db); - db_ids.push(catalog_db_ids[i]); - owners.push( - user_api - .get_ownership(&tenant, &OwnershipObject::Database { - catalog_name: catalog.clone(), - db_id: catalog_db_ids[i], - }) - .await - .ok() - .and_then(|ownership| ownership.map(|o| o.role.clone())), - ); - } - } - Err(err) => { - let msg = format!("Failed to get database: {}, {}", ctl.name(), err); - warn!("{}", msg); - } + catalog_db_names.extend(databases.into_iter().flatten()); + } else { + let msg = format!("Failed to get database name by id: {}", ctl.name()); + warn!("{}", msg); + } + + let db_idents = catalog_db_names + .iter() + .map(|name| DatabaseNameIdent::new(&tenant, name)) + .collect::>(); + let dbs = ctl.mget_databases(&tenant, &db_idents).await?; + for db in dbs { + catalog_names.push(catalog.clone()); + db_names.push(db.get_db_info().name_ident.database_name().to_string()); + let db_id = db.get_db_info().database_id.db_id; + db_ids.push(db_id); + owners.push( + user_api + .get_ownership(&tenant, &OwnershipObject::Database { + catalog_name: catalog.to_string(), + db_id, + }) + .await + .ok() + .and_then(|ownership| ownership.map(|o| o.role.clone())), + ); } } } else { diff --git a/src/query/storages/system/src/tables_table.rs b/src/query/storages/system/src/tables_table.rs index da2c547b02b1..0fd595579d54 100644 --- a/src/query/storages/system/src/tables_table.rs +++ b/src/query/storages/system/src/tables_table.rs @@ -39,6 +39,7 @@ use databend_common_expression::TableSchemaRef; use databend_common_expression::TableSchemaRefExt; use databend_common_functions::BUILTIN_FUNCTIONS; use databend_common_meta_app::principal::OwnershipObject; +use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::TableIdent; use databend_common_meta_app::schema::TableInfo; use databend_common_meta_app::schema::TableMeta; @@ -323,6 +324,7 @@ where TablesTable: HistoryAware ); } } + let catalog_dbs = visibility_checker.get_visibility_database(); for (ctl_name, ctl) in ctls.iter() { if let Some(push_downs) = &push_downs { @@ -356,15 +358,49 @@ where TablesTable: HistoryAware } if dbs.is_empty() || invalid_optimize { - dbs = match ctl.list_databases(&tenant).await { - Ok(dbs) => dbs, - Err(err) => { - let msg = - format!("List databases failed on catalog {}: {}", ctl.name(), err); - warn!("{}", msg); - ctx.push_warning(msg); - - vec![] + // None means has global level privileges + dbs = if let Some(catalog_dbs) = &catalog_dbs { + let mut final_dbs = vec![]; + for (catalog_name, dbs) in catalog_dbs { + if ctl.name() == catalog_name.to_string() { + let mut catalog_db_ids = vec![]; + let mut catalog_db_names = vec![]; + catalog_db_names.extend( + dbs.iter() + .filter_map(|(db_name, _)| *db_name) + .map(|db_name| db_name.to_string()), + ); + catalog_db_ids.extend(dbs.iter().filter_map(|(_, db_id)| *db_id)); + if let Ok(databases) = ctl + .mget_database_names_by_ids(&tenant, &catalog_db_ids) + .await + { + catalog_db_names.extend(databases.into_iter().flatten()); + } else { + let msg = + format!("Failed to get database name by id: {}", ctl.name()); + warn!("{}", msg); + } + let db_idents = catalog_db_names + .iter() + .map(|name| DatabaseNameIdent::new(&tenant, name)) + .collect::>(); + let dbs = ctl.mget_databases(&tenant, &db_idents).await?; + final_dbs.extend(dbs); + } + } + final_dbs + } else { + match ctl.list_databases(&tenant).await { + Ok(dbs) => dbs, + Err(err) => { + let msg = + format!("List databases failed on catalog {}: {}", ctl.name(), err); + warn!("{}", msg); + ctx.push_warning(msg); + + vec![] + } } } } diff --git a/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.result b/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.result index 9683d2d30155..0e6403f6038f 100644 --- a/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.result +++ b/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.result @@ -23,5 +23,8 @@ keywords VARCHAR NO NULL NULL reserved TINYINT UNSIGNED NO NULL NULL Error: APIError: ResponseError with 1063: Permission denied: User 'a'@'%' does not have the required privileges for database 'nogrant' Error: APIError: ResponseError with 1063: Permission denied: User 'a'@'%' does not have the required privileges for table 'nogrant.t' +=== grant system to a === +0 +0 1 0 diff --git a/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.sh b/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.sh index 945c936388f3..482999a37cda 100755 --- a/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.sh +++ b/tests/suites/0_stateless/18_rbac/18_0013_column_privilege.sh @@ -33,16 +33,20 @@ echo "use grant_db" | $USER_A_CONNECT echo "=== show columns ===" echo "show columns from one from system" | $USER_A_CONNECT echo "show columns from t from grant_db" | $USER_A_CONNECT -### will return err echo "show columns from roles from system" | $USER_A_CONNECT echo "show columns from keywords from information_schema" | $USER_A_CONNECT echo "show tables from nogrant" | $USER_A_CONNECT echo "show columns from t from nogrant" | $USER_A_CONNECT +echo "=== grant system to a ===" +echo "grant select on system.* to a" | $BENDSQL_CLIENT_CONNECT +echo "show tables from system" | $USER_A_CONNECT | echo $? +echo "use system" | $USER_A_CONNECT | echo $? + echo "select count(1) from information_schema.columns where table_schema in ('grant_db');" | $USER_A_CONNECT echo "select count(1) from information_schema.columns where table_schema in ('nogrant');" | $USER_A_CONNECT echo "drop database nogrant" | $BENDSQL_CLIENT_CONNECT echo "drop database grant_db" | $BENDSQL_CLIENT_CONNECT echo "drop table default.test_t" | $BENDSQL_CLIENT_CONNECT -echo "drop user a" | $BENDSQL_CLIENT_CONNECT \ No newline at end of file +echo "drop user a" | $BENDSQL_CLIENT_CONNECT