From ed21fe16843aabd7be56da476a6e5a9249a49dc2 Mon Sep 17 00:00:00 2001 From: taichong Date: Tue, 10 Dec 2024 07:53:20 +0800 Subject: [PATCH] refactor: optimize query system.tables when query single table (#16869) optimize: optimize query system.tables when query single table --- Cargo.lock | 1 + .../it/servers/http/http_query_handlers.rs | 2 +- .../it/storages/testdata/columns_table.txt | 4 + src/query/storages/system/Cargo.toml | 1 + src/query/storages/system/src/tables_table.rs | 403 +++++++++++------- 5 files changed, 250 insertions(+), 161 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 481499022f54..b5fe31ae7ab2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4530,6 +4530,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/query/service/tests/it/servers/http/http_query_handlers.rs b/src/query/service/tests/it/servers/http/http_query_handlers.rs index 76bbee3e9f42..39a81eb445b2 100644 --- a/src/query/service/tests/it/servers/http/http_query_handlers.rs +++ b/src/query/service/tests/it/servers/http/http_query_handlers.rs @@ -290,7 +290,7 @@ async fn test_simple_sql() -> Result<()> { assert_eq!(result.state, ExecuteStateKind::Succeeded, "{:?}", result); assert_eq!(result.next_uri, Some(final_uri.clone()), "{:?}", result); assert_eq!(result.data.len(), 10, "{:?}", result); - assert_eq!(result.schema.len(), 22, "{:?}", result); + assert_eq!(result.schema.len(), 23, "{:?}", result); // get state let uri = result.stats_uri.unwrap(); diff --git a/src/query/service/tests/it/storages/testdata/columns_table.txt b/src/query/service/tests/it/storages/testdata/columns_table.txt index 745b091b3c1a..b1e737fc0ba4 100644 --- a/src/query/service/tests/it/storages/testdata/columns_table.txt +++ b/src/query/service/tests/it/storages/testdata/columns_table.txt @@ -134,6 +134,10 @@ DB.Table: 'system'.'columns', Table: columns-table_id:1, ver:0, Engine: SystemCo | 'database_id' | 'system' | 'background_tasks' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | | 'database_id' | 'system' | 'databases' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | | 'database_id' | 'system' | 'databases_with_history' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | +| 'database_id' | 'system' | 'tables' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | +| 'database_id' | 'system' | 'tables_with_history' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | +| 'database_id' | 'system' | 'views' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | +| 'database_id' | 'system' | 'views_with_history' | 'UInt64' | 'BIGINT UNSIGNED' | '' | '' | 'NO' | '' | | 'databases' | 'system' | 'query_log' | 'String' | 'VARCHAR' | '' | '' | 'NO' | '' | | 'datetime_precision' | 'information_schema' | 'columns' | 'NULL' | 'NULL' | '' | '' | 'NO' | '' | | 'default' | 'information_schema' | 'columns' | 'String' | 'VARCHAR' | '' | '' | 'NO' | '' | diff --git a/src/query/storages/system/Cargo.toml b/src/query/storages/system/Cargo.toml index d36f47e0c688..3b31c6c3966f 100644 --- a/src/query/storages/system/Cargo.toml +++ b/src/query/storages/system/Cargo.toml @@ -25,6 +25,7 @@ databend-common-config = { workspace = true } databend-common-exception = { workspace = true } databend-common-expression = { workspace = true } databend-common-functions = { workspace = true } +databend-common-management = { workspace = true } databend-common-meta-api = { workspace = true } databend-common-meta-app = { workspace = true } databend-common-meta-types = { workspace = true } diff --git a/src/query/storages/system/src/tables_table.rs b/src/query/storages/system/src/tables_table.rs index 9d427bc696d3..44a07b6e9110 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(false).await?; - - self.get_full_data_from_catalogs(ctx, push_downs, catalogs, visibility_checker) + self.get_full_data_from_catalogs(ctx, push_downs, catalogs) .await } } @@ -144,6 +142,7 @@ where TablesTable: HistoryAware TableSchemaRefExt::create(vec![ TableField::new("catalog", TableDataType::String), TableField::new("database", TableDataType::String), + TableField::new("database_id", TableDataType::Number(NumberDataType::UInt64)), TableField::new("name", TableDataType::String), TableField::new("table_id", TableDataType::Number(NumberDataType::UInt64)), TableField::new( @@ -208,6 +207,7 @@ where TablesTable: HistoryAware TableSchemaRefExt::create(vec![ TableField::new("catalog", TableDataType::String), TableField::new("database", TableDataType::String), + TableField::new("database_id", TableDataType::Number(NumberDataType::UInt64)), TableField::new("name", TableDataType::String), TableField::new("table_id", TableDataType::Number(NumberDataType::UInt64)), TableField::new("engine", TableDataType::String), @@ -237,7 +237,6 @@ where TablesTable: HistoryAware ctx: Arc, push_downs: Option, catalogs: Vec>, - visibility_checker: GrantObjectVisibilityChecker, ) -> Result { let tenant = ctx.get_tenant(); @@ -246,6 +245,7 @@ where TablesTable: HistoryAware let mut catalogs = vec![]; let mut databases = vec![]; + let mut databases_ids = vec![]; let mut database_tables = vec![]; let mut owner: Vec> = Vec::new(); @@ -328,203 +328,284 @@ 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); } } + } + 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()); + databases_ids.push(db.get_db_info().database_id.db_id); + 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()); + databases_ids.push(db.get_db_info().database_id.db_id); + 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()); databases.push(db_name.to_owned()); + databases_ids.push(db.get_db_info().database_id.db_id); database_tables.push(table); if ownership.is_empty() { owner.push(None); @@ -684,6 +765,7 @@ where TablesTable: HistoryAware Ok(DataBlock::new_from_columns(vec![ StringType::from_data(catalogs), StringType::from_data(databases), + UInt64Type::from_data(databases_ids), StringType::from_data(names), UInt64Type::from_data(table_id), UInt64Type::from_data(total_columns), @@ -709,6 +791,7 @@ where TablesTable: HistoryAware Ok(DataBlock::new_from_columns(vec![ StringType::from_data(catalogs), StringType::from_data(databases), + UInt64Type::from_data(databases_ids), StringType::from_data(names), UInt64Type::from_data(table_id), StringType::from_data(engines),