From 9e8ef8b9599b8f8283e998e1f1884bf6ca3b9b95 Mon Sep 17 00:00:00 2001 From: taichong Date: Tue, 19 Nov 2024 13:42:55 +0800 Subject: [PATCH] optimize: optimize query system.tables when query single table --- Cargo.lock | 1 + src/meta/app/src/principal/mod.rs | 1 + src/meta/app/src/principal/user_grant.rs | 27 ++ src/query/catalog/src/table_context.rs | 5 +- src/query/management/src/role/role_mgr.rs | 1 + .../interpreters/access/privilege_access.rs | 28 +- src/query/service/src/sessions/query_ctx.rs | 10 +- src/query/service/src/sessions/session.rs | 9 +- .../src/sessions/session_privilege_mgr.rs | 41 +- .../table_functions/infer_schema/parquet.rs | 2 +- .../inspect_parquet/inspect_parquet_table.rs | 2 +- .../list_stage/list_stage_table.rs | 2 +- .../show_grants/show_grants_table.rs | 2 +- .../tests/it/sql/exec/get_table_bind_test.rs | 5 +- .../it/storages/fuse/operations/commit.rs | 5 +- src/query/storages/system/Cargo.toml | 1 + .../storages/system/src/columns_table.rs | 2 +- .../storages/system/src/databases_table.rs | 2 +- .../storages/system/src/indexes_table.rs | 2 +- src/query/storages/system/src/stages_table.rs | 2 +- .../storages/system/src/streams_table.rs | 2 +- src/query/storages/system/src/tables_table.rs | 408 +++++++++++------- .../system/src/user_functions_table.rs | 2 +- 23 files changed, 341 insertions(+), 221 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61c501affe8ad..ac64143912fe3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4563,6 +4563,7 @@ dependencies = [ "databend-common-exception", "databend-common-expression", "databend-common-functions", + "databend-common-management", "databend-common-meta-api", "databend-common-meta-app", "databend-common-meta-types", diff --git a/src/meta/app/src/principal/mod.rs b/src/meta/app/src/principal/mod.rs index 80fc40dcf4ca2..5682530411649 100644 --- a/src/meta/app/src/principal/mod.rs +++ b/src/meta/app/src/principal/mod.rs @@ -102,6 +102,7 @@ pub use user_defined_function::UserDefinedFunction; pub use user_grant::GrantEntry; pub use user_grant::GrantObject; pub use user_grant::UserGrantSet; +pub use user_grant::SYSTEM_TABLES_ALLOW_LIST; pub use user_identity::UserIdentity; pub use user_info::UserInfo; pub use user_info::UserOption; diff --git a/src/meta/app/src/principal/user_grant.rs b/src/meta/app/src/principal/user_grant.rs index 7ba3df94a0844..e08d864954737 100644 --- a/src/meta/app/src/principal/user_grant.rs +++ b/src/meta/app/src/principal/user_grant.rs @@ -21,6 +21,33 @@ use enumflags2::BitFlags; use crate::principal::UserPrivilegeSet; use crate::principal::UserPrivilegeType; +// some statements like `SELECT 1`, `SHOW USERS`, `SHOW ROLES`, `SHOW TABLES` will be +// rewritten to the queries on the system tables, we need to skip the privilege check on +// these tables. +pub const SYSTEM_TABLES_ALLOW_LIST: [&str; 21] = [ + "catalogs", + "columns", + "databases", + "databases_with_history", + "dictionaries", + "tables", + "views", + "tables_with_history", + "views_with_history", + "password_policies", + "streams", + "streams_terse", + "virtual_columns", + "users", + "roles", + "stages", + "one", + "processes", + "user_functions", + "functions", + "indexes", +]; + #[derive(serde::Serialize, serde::Deserialize, Clone, Debug, Eq, PartialEq, Hash)] pub enum GrantObject { Global, diff --git a/src/query/catalog/src/table_context.rs b/src/query/catalog/src/table_context.rs index 9ff28e69347ee..5b3fec7289815 100644 --- a/src/query/catalog/src/table_context.rs +++ b/src/query/catalog/src/table_context.rs @@ -224,7 +224,10 @@ pub trait TableContext: Send + Sync { check_current_role_only: bool, ) -> Result<()>; async fn get_available_roles(&self) -> Result>; - async fn get_visibility_checker(&self) -> Result; + async fn get_visibility_checker( + &self, + ignore_ownership: bool, + ) -> Result; fn get_fuse_version(&self) -> String; fn get_format_settings(&self) -> Result; fn get_tenant(&self) -> Tenant; diff --git a/src/query/management/src/role/role_mgr.rs b/src/query/management/src/role/role_mgr.rs index 81db047555c77..7f0fd6d88f3f9 100644 --- a/src/query/management/src/role/role_mgr.rs +++ b/src/query/management/src/role/role_mgr.rs @@ -211,6 +211,7 @@ impl RoleApi for RoleMgr { #[async_backtrace::framed] #[fastrace::trace] async fn get_ownerships(&self) -> Result>, ErrorCode> { + println!("will call get_ownerships"); let object_owner_prefix = self.ownership_object_prefix(); let values = self .kv_api diff --git a/src/query/service/src/interpreters/access/privilege_access.rs b/src/query/service/src/interpreters/access/privilege_access.rs index 95d538266d3e2..3281a19ef7426 100644 --- a/src/query/service/src/interpreters/access/privilege_access.rs +++ b/src/query/service/src/interpreters/access/privilege_access.rs @@ -29,6 +29,7 @@ use databend_common_meta_app::principal::StageType; use databend_common_meta_app::principal::UserGrantSet; use databend_common_meta_app::principal::UserPrivilegeSet; use databend_common_meta_app::principal::UserPrivilegeType; +use databend_common_meta_app::principal::SYSTEM_TABLES_ALLOW_LIST; use databend_common_meta_app::tenant::Tenant; use databend_common_meta_types::seq_value::SeqV; use databend_common_sql::binder::MutationType; @@ -58,33 +59,6 @@ enum ObjectId { Table(u64, u64), } -// some statements like `SELECT 1`, `SHOW USERS`, `SHOW ROLES`, `SHOW TABLES` will be -// rewritten to the queries on the system tables, we need to skip the privilege check on -// these tables. -const SYSTEM_TABLES_ALLOW_LIST: [&str; 21] = [ - "catalogs", - "columns", - "databases", - "databases_with_history", - "dictionaries", - "tables", - "views", - "tables_with_history", - "views_with_history", - "password_policies", - "streams", - "streams_terse", - "virtual_columns", - "users", - "roles", - "stages", - "one", - "processes", - "user_functions", - "functions", - "indexes", -]; - // table functions that need `Super` privilege const SYSTEM_TABLE_FUNCTIONS: [&str; 1] = ["fuse_amend"]; diff --git a/src/query/service/src/sessions/query_ctx.rs b/src/query/service/src/sessions/query_ctx.rs index c5822b6da492c..a8dd5e987cf50 100644 --- a/src/query/service/src/sessions/query_ctx.rs +++ b/src/query/service/src/sessions/query_ctx.rs @@ -692,8 +692,14 @@ impl TableContext for QueryContext { self.get_current_session().get_id() } - async fn get_visibility_checker(&self) -> Result { - self.shared.session.get_visibility_checker().await + async fn get_visibility_checker( + &self, + ignore_ownership: bool, + ) -> Result { + self.shared + .session + .get_visibility_checker(ignore_ownership) + .await } fn get_fuse_version(&self) -> String { diff --git a/src/query/service/src/sessions/session.rs b/src/query/service/src/sessions/session.rs index 7f1bedc157823..878224401f244 100644 --- a/src/query/service/src/sessions/session.rs +++ b/src/query/service/src/sessions/session.rs @@ -310,8 +310,13 @@ impl Session { } #[async_backtrace::framed] - pub async fn get_visibility_checker(&self) -> Result { - self.privilege_mgr().get_visibility_checker().await + pub async fn get_visibility_checker( + &self, + ignore_ownership: bool, + ) -> Result { + self.privilege_mgr() + .get_visibility_checker(ignore_ownership) + .await } pub fn get_settings(&self) -> Arc { diff --git a/src/query/service/src/sessions/session_privilege_mgr.rs b/src/query/service/src/sessions/session_privilege_mgr.rs index 8d772eb1c8676..914629cc3cd2b 100644 --- a/src/query/service/src/sessions/session_privilege_mgr.rs +++ b/src/query/service/src/sessions/session_privilege_mgr.rs @@ -75,7 +75,10 @@ pub trait SessionPrivilegeManager { async fn validate_available_role(&self, role_name: &str) -> Result; - async fn get_visibility_checker(&self) -> Result; + async fn get_visibility_checker( + &self, + ignore_ownership: bool, + ) -> Result; // fn show_grants(&self); } @@ -336,27 +339,31 @@ impl<'a> SessionPrivilegeManager for SessionPrivilegeManagerImpl<'a> { } #[async_backtrace::framed] - async fn get_visibility_checker(&self) -> Result { + async fn get_visibility_checker( + &self, + ignore_ownership: bool, + ) -> Result { // TODO(liyz): is it check the visibility according onwerships? - let user_api = UserApiProvider::instance(); - let ownerships = user_api - .role_api(&self.session_ctx.get_current_tenant()) - .get_ownerships() - .await?; let roles = self.get_all_effective_roles().await?; let roles_name: Vec = roles.iter().map(|role| role.name.to_string()).collect(); - let ownership_objects = if roles_name.contains(&"account_admin".to_string()) { - vec![] - } else { - let mut ownership_objects = vec![]; - for ownership in ownerships { - if roles_name.contains(&ownership.data.role) { - ownership_objects.push(ownership.data.object); + let ownership_objects = + if roles_name.contains(&"account_admin".to_string()) || ignore_ownership { + vec![] + } else { + let user_api = UserApiProvider::instance(); + let ownerships = user_api + .role_api(&self.session_ctx.get_current_tenant()) + .get_ownerships() + .await?; + let mut ownership_objects = vec![]; + for ownership in ownerships { + if roles_name.contains(&ownership.data.role) { + ownership_objects.push(ownership.data.object); + } } - } - ownership_objects - }; + ownership_objects + }; Ok(GrantObjectVisibilityChecker::new( &self.get_current_user()?, diff --git a/src/query/service/src/table_functions/infer_schema/parquet.rs b/src/query/service/src/table_functions/infer_schema/parquet.rs index de814c75c3957..5bcbd8eaf8c1d 100644 --- a/src/query/service/src/table_functions/infer_schema/parquet.rs +++ b/src/query/service/src/table_functions/infer_schema/parquet.rs @@ -99,7 +99,7 @@ impl AsyncSource for ParquetInferSchemaSource { .get_settings() .get_enable_experimental_rbac_check()?; if enable_experimental_rbac_check { - let visibility_checker = self.ctx.get_visibility_checker().await?; + let visibility_checker = self.ctx.get_visibility_checker(false).await?; if !(stage_info.is_temporary || visibility_checker.check_stage_read_visibility(&stage_info.stage_name) || stage_info.stage_type == StageType::User diff --git a/src/query/service/src/table_functions/inspect_parquet/inspect_parquet_table.rs b/src/query/service/src/table_functions/inspect_parquet/inspect_parquet_table.rs index a0e5e13f2baaa..e483df46c8a7f 100644 --- a/src/query/service/src/table_functions/inspect_parquet/inspect_parquet_table.rs +++ b/src/query/service/src/table_functions/inspect_parquet/inspect_parquet_table.rs @@ -213,7 +213,7 @@ impl AsyncSource for InspectParquetSource { .get_settings() .get_enable_experimental_rbac_check()?; if enable_experimental_rbac_check { - let visibility_checker = self.ctx.get_visibility_checker().await?; + let visibility_checker = self.ctx.get_visibility_checker(false).await?; if !(stage_info.is_temporary || visibility_checker.check_stage_read_visibility(&stage_info.stage_name) || stage_info.stage_type == StageType::User diff --git a/src/query/service/src/table_functions/list_stage/list_stage_table.rs b/src/query/service/src/table_functions/list_stage/list_stage_table.rs index 89c3e8e45a0fa..d8f51289553e2 100644 --- a/src/query/service/src/table_functions/list_stage/list_stage_table.rs +++ b/src/query/service/src/table_functions/list_stage/list_stage_table.rs @@ -187,7 +187,7 @@ impl ListStagesSource { .get_settings() .get_enable_experimental_rbac_check()?; if enable_experimental_rbac_check { - let visibility_checker = self.ctx.get_visibility_checker().await?; + let visibility_checker = self.ctx.get_visibility_checker(false).await?; if !(stage_info.is_temporary || visibility_checker.check_stage_read_visibility(&stage_info.stage_name) || stage_info.stage_type == StageType::User diff --git a/src/query/service/src/table_functions/show_grants/show_grants_table.rs b/src/query/service/src/table_functions/show_grants/show_grants_table.rs index d6357c0d1a5d0..8976ce72dafe4 100644 --- a/src/query/service/src/table_functions/show_grants/show_grants_table.rs +++ b/src/query/service/src/table_functions/show_grants/show_grants_table.rs @@ -566,7 +566,7 @@ async fn show_object_grant( let tenant = ctx.get_tenant(); let user_api = UserApiProvider::instance(); let roles = user_api.get_roles(&tenant).await?; - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; let current_user = ctx.get_current_user()?.identity().username; let (object, owner_object, object_id, object_name) = match grant_type { "table" => { 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 16a8824240a89..ea8d1476bc252 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 @@ -690,7 +690,10 @@ impl TableContext for CtxDelegation { todo!() } - async fn get_visibility_checker(&self) -> Result { + async fn get_visibility_checker( + &self, + _ignore_ownership: bool, + ) -> Result { todo!() } 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 94b3f39457c9e..02db1fb9ffbf7 100644 --- a/src/query/service/tests/it/storages/fuse/operations/commit.rs +++ b/src/query/service/tests/it/storages/fuse/operations/commit.rs @@ -594,7 +594,10 @@ impl TableContext for CtxDelegation { todo!() } - async fn get_visibility_checker(&self) -> Result { + async fn get_visibility_checker( + &self, + _ignore_ownership: bool, + ) -> Result { todo!() } diff --git a/src/query/storages/system/Cargo.toml b/src/query/storages/system/Cargo.toml index 31b4e329688c4..0c63e3b04f03e 100644 --- a/src/query/storages/system/Cargo.toml +++ b/src/query/storages/system/Cargo.toml @@ -39,6 +39,7 @@ databend-common-storages-result-cache = { workspace = true } databend-common-storages-stream = { workspace = true } databend-common-storages-view = { workspace = true } databend-common-users = { workspace = true } +databend-common-management = { workspace = true } databend-storages-common-cache = { workspace = true } futures = { workspace = true } itertools = { workspace = true } diff --git a/src/query/storages/system/src/columns_table.rs b/src/query/storages/system/src/columns_table.rs index 094a01613f165..5d9b2cf296df0 100644 --- a/src/query/storages/system/src/columns_table.rs +++ b/src/query/storages/system/src/columns_table.rs @@ -274,7 +274,7 @@ pub(crate) async fn dump_tables( } } - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; let mut final_dbs: Vec<(String, u64)> = Vec::new(); diff --git a/src/query/storages/system/src/databases_table.rs b/src/query/storages/system/src/databases_table.rs index 4adbbb880d73f..b2284a96790a0 100644 --- a/src/query/storages/system/src/databases_table.rs +++ b/src/query/storages/system/src/databases_table.rs @@ -117,7 +117,7 @@ where DatabasesTable: HistoryAware let mut owners: Vec> = vec![]; let mut dropped_on: Vec> = vec![]; - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; let catalog_dbs = visibility_checker.get_visibility_database(); // None means has global level privileges if let Some(catalog_dbs) = catalog_dbs { diff --git a/src/query/storages/system/src/indexes_table.rs b/src/query/storages/system/src/indexes_table.rs index 47daad8deebfa..d133aa931e8b4 100644 --- a/src/query/storages/system/src/indexes_table.rs +++ b/src/query/storages/system/src/indexes_table.rs @@ -154,7 +154,7 @@ impl IndexesTable { ctx: Arc, ) -> Result> { let tenant = ctx.get_tenant(); - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; let catalog = ctx.get_catalog(CATALOG_DEFAULT).await?; let ctl_name = catalog.name(); diff --git a/src/query/storages/system/src/stages_table.rs b/src/query/storages/system/src/stages_table.rs index 91a09bfaacbb9..13a03d1ed9fa6 100644 --- a/src/query/storages/system/src/stages_table.rs +++ b/src/query/storages/system/src/stages_table.rs @@ -62,7 +62,7 @@ impl AsyncSystemTable for StagesTable { let enable_experimental_rbac_check = ctx.get_settings().get_enable_experimental_rbac_check()?; let stages = if enable_experimental_rbac_check { - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; stages .into_iter() .filter(|stage| { diff --git a/src/query/storages/system/src/streams_table.rs b/src/query/storages/system/src/streams_table.rs index 6bf7013a1652f..6148391123770 100644 --- a/src/query/storages/system/src/streams_table.rs +++ b/src/query/storages/system/src/streams_table.rs @@ -80,7 +80,7 @@ impl AsyncSystemTable for StreamsTable { .iter() .map(|e| (e.name(), e.clone())) .collect::>(); - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; let user_api = UserApiProvider::instance(); let mut catalogs = vec![]; diff --git a/src/query/storages/system/src/tables_table.rs b/src/query/storages/system/src/tables_table.rs index 0817f756e5f1a..7712b25e992bc 100644 --- a/src/query/storages/system/src/tables_table.rs +++ b/src/query/storages/system/src/tables_table.rs @@ -38,6 +38,7 @@ use databend_common_expression::TableField; use databend_common_expression::TableSchemaRef; use databend_common_expression::TableSchemaRefExt; use databend_common_functions::BUILTIN_FUNCTIONS; +use databend_common_management::RoleApi; use databend_common_meta_app::principal::OwnershipObject; use databend_common_meta_app::schema::database_name_ident::DatabaseNameIdent; use databend_common_meta_app::schema::TableIdent; @@ -46,7 +47,6 @@ use databend_common_meta_app::schema::TableMeta; use databend_common_meta_app::tenant::Tenant; use databend_common_storages_fuse::FuseTable; use databend_common_storages_view::view_table::QUERY; -use databend_common_users::GrantObjectVisibilityChecker; use databend_common_users::UserApiProvider; use log::warn; @@ -129,9 +129,7 @@ where TablesTable: HistoryAware .into_iter() .map(|cat| cat.disable_table_info_refresh()) .collect::>>()?; - let visibility_checker = ctx.get_visibility_checker().await?; - - self.get_full_data_from_catalogs(ctx, push_downs, catalogs, visibility_checker) + self.get_full_data_from_catalogs(ctx, push_downs, catalogs) .await } } @@ -237,7 +235,6 @@ where TablesTable: HistoryAware ctx: Arc, push_downs: Option, catalogs: Vec>, - visibility_checker: GrantObjectVisibilityChecker, ) -> Result { let tenant = ctx.get_tenant(); @@ -328,199 +325,290 @@ 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 { - if push_downs.filters.as_ref().map(|f| &f.filter).is_some() { - for db in &db_name { - match ctl.get_database(&tenant, db.as_str()).await { - Ok(database) => dbs.push(database), - Err(err) => { - let msg = format!("Failed to get database: {}, {}", db, err); - warn!("{}", msg); - } + + // from system.tables where database = 'db' and name = 'name' + // from system.tables where database = 'db' and table_id = 123 + if db_name.len() == 1 + && !invalid_optimize + && tables_names.len() + tables_ids.len() == 1 + && !invalid_tables_ids + && !WITH_HISTORY + { + let visibility_checker = ctx.get_visibility_checker(true).await?; + for (ctl_name, ctl) in ctls.iter() { + for db in &db_name { + match ctl.get_database(&tenant, db.as_str()).await { + Ok(database) => dbs.push(database), + Err(err) => { + let msg = format!("Failed to get database: {}, {}", db, err); + warn!("{}", msg); } } + } + // match ctl.mget_table_names_by_ids(&tenant, &tables_ids).await { + // Ok(tables) => { + // for table in tables.into_iter().flatten() { + // if !tables_names.contains(&table) { + // tables_names.push(table.clone()); + // } + // } + // } + // Err(err) => { + // let msg = format!("Failed to get tables: {}, {}", ctl.name(), err); + // warn!("{}", msg); + // } + // } + if let Err(err) = ctl.mget_table_names_by_ids(&tenant, &tables_ids).await { + warn!("Failed to get tables: {}, {}", ctl.name(), err); + } else { + let new_tables_names = ctl + .mget_table_names_by_ids(&tenant, &tables_ids) + .await? + .into_iter() + .flatten() + .filter(|table| !tables_names.contains(table)) + .collect::>(); + tables_names.extend(new_tables_names); + } - if !WITH_HISTORY { - match ctl.mget_table_names_by_ids(&tenant, &tables_ids).await { - Ok(tables) => { - for table in tables.into_iter().flatten() { - if !tables_names.contains(&table) { - tables_names.push(table.clone()); + for table_name in &tables_names { + for db in &dbs { + match ctl.get_table(&tenant, db.name(), table_name).await { + Ok(t) => { + let db_id = db.get_db_info().database_id.db_id; + let table_id = t.get_id(); + let role = user_api + .role_api(&tenant) + .get_ownership(&OwnershipObject::Table { + catalog_name: ctl_name.to_string(), + db_id, + table_id, + }) + .await? + .map(|o| o.role); + if visibility_checker.check_table_visibility( + ctl_name, + db.name(), + table_name, + db_id, + t.get_id(), + ) { + catalogs.push(ctl_name.as_str()); + databases.push(db.name().to_owned()); + database_tables.push(t); + owner.push(role); + } else if let Some(role) = role { + let roles = ctx.get_all_effective_roles().await?; + if roles.iter().any(|r| r.name == role) { + catalogs.push(ctl_name.as_str()); + databases.push(db.name().to_owned()); + database_tables.push(t); + owner.push(Some(role)); } } } Err(err) => { - let msg = format!("Failed to get tables: {}, {}", ctl.name(), err); + let msg = format!( + "Failed to get table in database: {}, {}", + db.name(), + err + ); + // warn no need to pad in ctx warn!("{}", msg); + continue; } } } } } - - if dbs.is_empty() || invalid_optimize { - // 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); + } else { + let visibility_checker = ctx.get_visibility_checker(false).await?; + let catalog_dbs = visibility_checker.get_visibility_database(); + + for (ctl_name, ctl) in ctls.iter() { + if let Some(push_downs) = &push_downs { + if push_downs.filters.as_ref().map(|f| &f.filter).is_some() { + for db in &db_name { + match ctl.get_database(&tenant, db.as_str()).await { + Ok(database) => dbs.push(database), + Err(err) => { + let msg = format!("Failed to get database: {}, {}", db, err); + 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![] + if !WITH_HISTORY { + match ctl.mget_table_names_by_ids(&tenant, &tables_ids).await { + Ok(tables) => { + for table in tables.into_iter().flatten() { + if !tables_names.contains(&table) { + tables_names.push(table.clone()); + } + } + } + Err(err) => { + let msg = + format!("Failed to get tables: {}, {}", ctl.name(), err); + warn!("{}", msg); + } + } } } } - } - let final_dbs = dbs - .clone() - .into_iter() - .filter(|db| { - visibility_checker.check_database_visibility( - ctl_name, - db.name(), - db.get_db_info().database_id.db_id, - ) - }) - .collect::>(); - - let ownership = if get_ownership { - user_api.get_ownerships(&tenant).await.unwrap_or_default() - } else { - HashMap::new() - }; - for db in final_dbs { - let db_id = db.get_db_info().database_id.db_id; - let db_name = db.name(); - let tables = if tables_names.is_empty() - || tables_names.len() > 10 - || invalid_tables_ids - || invalid_optimize - { - match Self::list_tables(ctl, &tenant, db_name, WITH_HISTORY, WITHOUT_VIEW).await - { - Ok(tables) => tables, - Err(err) => { - // swallow the errors related with remote database or tables, avoid ANY of bad table config corrupt ALL of the results. - // these databases might be: - // - sharing database - // - hive database - // - iceberg database - // - others - // TODO(liyz): return the warnings in the HTTP query protocol. - let msg = - format!("Failed to list tables in database: {}, {}", db_name, err); - warn!("{}", msg); - ctx.push_warning(msg); - - continue; + if dbs.is_empty() || invalid_optimize { + // 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); + } } - } - } else if WITH_HISTORY { - // Only can call get_table - let mut tables = Vec::new(); - for table_name in &tables_names { - match ctl.get_table_history(&tenant, db_name, table_name).await { - Ok(t) => tables.extend(t), + final_dbs + } else { + match ctl.list_databases(&tenant).await { + Ok(dbs) => dbs, Err(err) => { let msg = format!( - "Failed to get_table_history tables in database: {}, {}", - db_name, err + "List databases failed on catalog {}: {}", + ctl.name(), + err ); - // warn no need to pad in ctx warn!("{}", msg); - continue; + ctx.push_warning(msg); + + vec![] } } } - tables + } + + let final_dbs = dbs + .clone() + .into_iter() + .filter(|db| { + visibility_checker.check_database_visibility( + ctl_name, + db.name(), + db.get_db_info().database_id.db_id, + ) + }) + .collect::>(); + + let ownership = if get_ownership { + user_api.get_ownerships(&tenant).await.unwrap_or_default() } else { - // Only can call get_table - let mut tables = Vec::new(); - for table_name in &tables_names { - match ctl.get_table(&tenant, db_name, table_name).await { - Ok(t) => tables.push(t), + HashMap::new() + }; + for db in final_dbs { + let db_id = db.get_db_info().database_id.db_id; + let db_name = db.name(); + let tables = if tables_names.is_empty() + || tables_names.len() > 10 + || invalid_tables_ids + || invalid_optimize + { + match Self::list_tables(ctl, &tenant, db_name, WITH_HISTORY, WITHOUT_VIEW) + .await + { + Ok(tables) => tables, Err(err) => { + // swallow the errors related with remote database or tables, avoid ANY of bad table config corrupt ALL of the results. + // these databases might be: + // - sharing database + // - hive database + // - iceberg database + // - others + // TODO(liyz): return the warnings in the HTTP query protocol. let msg = format!( - "Failed to get table in database: {}, {}", + "Failed to list tables in database: {}, {}", db_name, err ); - // warn no need to pad in ctx warn!("{}", msg); + ctx.push_warning(msg); + continue; } } - } - tables - }; - - for table in tables { - let table_id = table.get_id(); - // If db1 is visible, do not mean db1.table1 is visible. A user may have a grant about db1.table2, so db1 is visible - // for her, but db1.table1 may be not visible. So we need an extra check about table here after db visibility check. - if visibility_checker.check_table_visibility( - ctl_name, - db_name, - table.name(), - db_id, - table_id, - ) && !table.is_stream() - { - if !WITHOUT_VIEW && table.get_table_info().engine() == "VIEW" { - catalogs.push(ctl_name.as_str()); - databases.push(db_name.to_owned()); - database_tables.push(table); - if ownership.is_empty() { - owner.push(None); - } else { - owner.push( - ownership - .get(&OwnershipObject::Table { - catalog_name: ctl_name.to_string(), - db_id, - table_id, - }) - .map(|role| role.to_string()), - ); + } else if WITH_HISTORY { + // Only can call get_table + let mut tables = Vec::new(); + for table_name in &tables_names { + match ctl.get_table_history(&tenant, db_name, table_name).await { + Ok(t) => tables.extend(t), + Err(err) => { + let msg = format!( + "Failed to get_table_history tables in database: {}, {}", + db_name, err + ); + // warn no need to pad in ctx + warn!("{}", msg); + continue; + } } - } else if WITHOUT_VIEW { + } + tables + } else { + // Only can call get_table + let mut tables = Vec::new(); + for table_name in &tables_names { + match ctl.get_table(&tenant, db_name, table_name).await { + Ok(t) => tables.push(t), + Err(err) => { + let msg = format!( + "Failed to get table in database: {}, {}", + db_name, err + ); + // warn no need to pad in ctx + warn!("{}", msg); + continue; + } + } + } + tables + }; + + for table in tables { + let table_id = table.get_id(); + // If db1 is visible, do not mean db1.table1 is visible. A user may have a grant about db1.table2, so db1 is visible + // for her, but db1.table1 may be not visible. So we need an extra check about table here after db visibility check. + if (table.get_table_info().engine() == "VIEW" || WITHOUT_VIEW) + && !table.is_stream() + && visibility_checker.check_table_visibility( + ctl_name, + db_name, + table.name(), + db_id, + table_id, + ) + { // system.tables store view name but not store view query // decrease information_schema.tables union. catalogs.push(ctl_name.as_str()); diff --git a/src/query/storages/system/src/user_functions_table.rs b/src/query/storages/system/src/user_functions_table.rs index b1580f15d48a6..dc6ca994ed054 100644 --- a/src/query/storages/system/src/user_functions_table.rs +++ b/src/query/storages/system/src/user_functions_table.rs @@ -60,7 +60,7 @@ impl AsyncSystemTable for UserFunctionsTable { let enable_experimental_rbac_check = ctx.get_settings().get_enable_experimental_rbac_check()?; let user_functions = if enable_experimental_rbac_check { - let visibility_checker = ctx.get_visibility_checker().await?; + let visibility_checker = ctx.get_visibility_checker(false).await?; let udfs = UserFunctionsTable::get_udfs(&ctx.get_tenant()).await?; udfs.into_iter() .filter(|udf| visibility_checker.check_udf_visibility(&udf.name))