From 5a0f93e05538aa82d6edc5aca2e43e91dfe4e11b Mon Sep 17 00:00:00 2001
From: Denis Smirnov <sd@picodata.io>
Date: Wed, 20 Apr 2022 12:03:16 +0700
Subject: [PATCH] fix: make engine reference immutable while query execution
---
benches/engine.rs | 4 ++
benches/parse.rs | 4 +-
src/executor.rs | 4 +-
src/executor/bucket/tests.rs | 20 +++++-----
src/executor/engine.rs | 3 ++
src/executor/engine/cartridge.rs | 7 ++--
.../engine/cartridge/cache/lru/tests.rs | 0
src/executor/engine/mock.rs | 15 ++++---
src/executor/tests.rs | 40 +++++++++----------
src/parser.rs | 29 +++++++++-----
10 files changed, 74 insertions(+), 52 deletions(-)
create mode 100644 src/executor/engine/cartridge/cache/lru/tests.rs
diff --git a/benches/engine.rs b/benches/engine.rs
index 96fc6f96e2..e9f38b7164 100644
--- a/benches/engine.rs
+++ b/benches/engine.rs
@@ -221,6 +221,10 @@ impl Engine for EngineMock {
self.metadata.tables.clear();
}
+ fn is_metadata_empty(&self) -> bool {
+ self.metadata.tables.is_empty()
+ }
+
fn get_schema(&self) -> Result<Option<String>, QueryPlannerError> {
Ok(Some("".to_string()))
}
diff --git a/benches/parse.rs b/benches/parse.rs
index d3b863c61f..557914c503 100644
--- a/benches/parse.rs
+++ b/benches/parse.rs
@@ -227,8 +227,8 @@ fn query1() {
) AS "t3"
WHERE
"reestrid" = 452842574"#;
- let mut engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let engine = EngineMock::new();
+ let mut query = Query::new(&engine, sql).unwrap();
let top_id = query.get_exec_plan().get_ir_plan().get_top().unwrap();
query.bucket_discovery(top_id).unwrap();
query.get_exec_plan().subtree_as_sql(top_id).unwrap();
diff --git a/src/executor.rs b/src/executor.rs
index b2644c0b72..0b9bb35b5d 100644
--- a/src/executor.rs
+++ b/src/executor.rs
@@ -62,7 +62,7 @@ where
/// Execution plan
exec_plan: ExecutionPlan,
/// Execution engine
- engine: &'a mut T,
+ engine: &'a T,
/// Bucket map
bucket_map: HashMap<usize, Buckets>,
}
@@ -78,7 +78,7 @@ where
/// - Failed to build AST.
/// - Failed to build IR plan.
/// - Failed to apply optimizing transformations to IR plan.
- pub fn new(engine: &'a mut T, sql: &str) -> Result<Self, QueryPlannerError>
+ pub fn new(engine: &'a T, sql: &str) -> Result<Self, QueryPlannerError>
where
T::Metadata: Metadata,
{
diff --git a/src/executor/bucket/tests.rs b/src/executor/bucket/tests.rs
index 8166cd2e20..53d5a1c9af 100644
--- a/src/executor/bucket/tests.rs
+++ b/src/executor/bucket/tests.rs
@@ -14,8 +14,8 @@ fn simple_union_query() {
) as "t3"
WHERE "id" = 1"#;
- let mut engine = EngineMock::new();
- let mut query = Query::new(&mut engine, query).unwrap();
+ let engine = EngineMock::new();
+ let mut query = Query::new(&engine, query).unwrap();
let plan = query.exec_plan.get_ir_plan();
let top = plan.get_top().unwrap();
let buckets = query.bucket_discovery(top).unwrap();
@@ -35,8 +35,8 @@ fn simple_disjunction_in_union_query() {
) as "t3"
WHERE ("id" = 1) OR ("id" = 100)"#;
- let mut engine = EngineMock::new();
- let mut query = Query::new(&mut engine, query).unwrap();
+ let engine = EngineMock::new();
+ let mut query = Query::new(&engine, query).unwrap();
let plan = query.exec_plan.get_ir_plan();
let top = plan.get_top().unwrap();
let buckets = query.bucket_discovery(top).unwrap();
@@ -61,8 +61,8 @@ fn complex_shard_key_union_query() {
WHERE "sys_op" > 1) AS "t3"
WHERE "identification_number" = 1 AND "product_code" = '222'"#;
- let mut engine = EngineMock::new();
- let mut query = Query::new(&mut engine, query).unwrap();
+ let engine = EngineMock::new();
+ let mut query = Query::new(&engine, query).unwrap();
let plan = query.exec_plan.get_ir_plan();
let top = plan.get_top().unwrap();
let buckets = query.bucket_discovery(top).unwrap();
@@ -90,8 +90,8 @@ fn union_complex_cond_query() {
AND ("product_code" = '222'
OR "product_code" = '111')"#;
- let mut engine = EngineMock::new();
- let mut query = Query::new(&mut engine, query).unwrap();
+ let engine = EngineMock::new();
+ let mut query = Query::new(&engine, query).unwrap();
let plan = query.exec_plan.get_ir_plan();
let top = plan.get_top().unwrap();
let buckets = query.bucket_discovery(top).unwrap();
@@ -123,8 +123,8 @@ fn union_query_conjunction() {
UNION ALL
SELECT * FROM "test_space_hist" WHERE "id" = 2"#;
- let mut engine = EngineMock::new();
- let mut query = Query::new(&mut engine, query).unwrap();
+ let engine = EngineMock::new();
+ let mut query = Query::new(&engine, query).unwrap();
let plan = query.exec_plan.get_ir_plan();
let top = plan.get_top().unwrap();
let buckets = query.bucket_discovery(top).unwrap();
diff --git a/src/executor/engine.rs b/src/executor/engine.rs
index d045c7646d..69ff881970 100644
--- a/src/executor/engine.rs
+++ b/src/executor/engine.rs
@@ -57,6 +57,9 @@ pub trait Engine {
/// Clear metadata information
fn clear_metadata(&mut self);
+ /// Check if the cache is empty.
+ fn is_metadata_empty(&self) -> bool;
+
/// Retrieve cluster schema.
///
/// # Errors
diff --git a/src/executor/engine/cartridge.rs b/src/executor/engine/cartridge.rs
index b4f3cd3005..badfa234a5 100644
--- a/src/executor/engine/cartridge.rs
+++ b/src/executor/engine/cartridge.rs
@@ -40,6 +40,10 @@ impl Engine for Runtime {
self.metadata = ClusterAppConfig::new();
}
+ fn is_metadata_empty(&self) -> bool {
+ self.metadata.is_empty()
+ }
+
fn get_schema(&self) -> Result<Option<String>, QueryPlannerError> {
if self.metadata.is_empty() {
let lua = tarantool::lua_state();
@@ -88,9 +92,6 @@ impl Engine for Runtime {
}
fn update_metadata(&mut self, schema: String, timeout: u64) -> Result<(), QueryPlannerError> {
- if !&self.metadata.is_empty() {
- return Ok(());
- }
self.metadata.load_schema(&schema)?;
self.metadata.set_exec_waiting_timeout(timeout);
Ok(())
diff --git a/src/executor/engine/cartridge/cache/lru/tests.rs b/src/executor/engine/cartridge/cache/lru/tests.rs
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/src/executor/engine/mock.rs b/src/executor/engine/mock.rs
index cccf7db143..7578566e08 100644
--- a/src/executor/engine/mock.rs
+++ b/src/executor/engine/mock.rs
@@ -1,3 +1,4 @@
+use std::cell::RefCell;
use std::collections::HashMap;
use crate::errors::QueryPlannerError;
@@ -144,7 +145,7 @@ impl MetadataMock {
#[derive(Debug, Clone)]
pub struct EngineMock {
metadata: MetadataMock,
- virtual_tables: HashMap<usize, VirtualTable>,
+ virtual_tables: RefCell<HashMap<usize, VirtualTable>>,
}
impl Engine for EngineMock {
@@ -161,6 +162,10 @@ impl Engine for EngineMock {
self.metadata.tables.clear();
}
+ fn is_metadata_empty(&self) -> bool {
+ self.metadata.tables.is_empty()
+ }
+
fn get_schema(&self) -> Result<Option<String>, QueryPlannerError> {
Ok(Some("".to_string()))
}
@@ -180,7 +185,7 @@ impl Engine for EngineMock {
motion_node_id: usize,
_buckets: &Buckets,
) -> Result<VirtualTable, QueryPlannerError> {
- if let Some(virtual_table) = self.virtual_tables.get(&motion_node_id) {
+ if let Some(virtual_table) = self.virtual_tables.borrow().get(&motion_node_id) {
Ok(virtual_table.clone())
} else {
Err(QueryPlannerError::CustomError(
@@ -248,13 +253,13 @@ impl EngineMock {
pub fn new() -> Self {
EngineMock {
metadata: MetadataMock::new(),
- virtual_tables: HashMap::new(),
+ virtual_tables: RefCell::new(HashMap::new()),
}
}
#[allow(dead_code)]
- pub fn add_virtual_table(&mut self, id: usize, table: VirtualTable) {
- self.virtual_tables.insert(id, table);
+ pub fn add_virtual_table(&self, id: usize, table: VirtualTable) {
+ self.virtual_tables.borrow_mut().insert(id, table);
}
}
diff --git a/src/executor/tests.rs b/src/executor/tests.rs
index 89cac9cedf..4bb20345de 100644
--- a/src/executor/tests.rs
+++ b/src/executor/tests.rs
@@ -11,9 +11,9 @@ use super::*;
#[test]
fn shard_query() {
let sql = r#"SELECT "FIRST_NAME" FROM "test_space" where "id" = 1"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let mut expected = BoxExecuteFormat::new();
let bucket = query.engine.determine_bucket_id("1");
@@ -39,9 +39,9 @@ fn shard_union_query() {
WHERE "sys_op" > 1) AS "t3"
WHERE "id" = 1"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let mut expected = BoxExecuteFormat::new();
let bucket = query.engine.determine_bucket_id("1");
@@ -69,9 +69,9 @@ fn shard_union_query() {
#[test]
fn map_reduce_query() {
let sql = r#"SELECT "product_code" FROM "hash_testing" where "identification_number" = 1 and "product_code" = '457'"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let mut expected = BoxExecuteFormat::new();
let bucket = query.engine.determine_bucket_id(&["1", "457"].join(""));
@@ -94,9 +94,9 @@ fn map_reduce_query() {
fn linker_test() {
let sql = r#"SELECT "FIRST_NAME" FROM "test_space" where "id" in
(SELECT "identification_number" FROM "hash_testing" where "identification_number" > 1)"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
let virtual_table = virtual_table_23();
query.engine.add_virtual_table(motion_id, virtual_table);
@@ -144,9 +144,9 @@ fn union_linker_test() {
) as "t2"
WHERE "product_code" = '123')"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
let virtual_table = virtual_table_23();
query.engine.add_virtual_table(motion_id, virtual_table);
@@ -223,9 +223,9 @@ INNER JOIN
ON "t3"."id" = "t8"."identification_number"
WHERE "t3"."id" = 2 AND "t8"."identification_number" = 2"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
let mut virtual_table = virtual_table_23();
virtual_table.set_alias("\"t8\"").unwrap();
@@ -273,9 +273,9 @@ fn join_linker2_test() {
select "id" as "id1", "id" as "id2" from "test_space_hist"
) as "t2" on "t1"."id" = 1"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
let mut virtual_table = VirtualTable::new();
@@ -325,9 +325,9 @@ fn join_linker3_test() {
(SELECT "id" as "id1", "FIRST_NAME" FROM "test_space") AS "t2"
ON "t2"."id1" = 1"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
let mut virtual_table = VirtualTable::new();
@@ -377,9 +377,9 @@ fn join_linker4_test() {
on t1."id" = t2."r_id" and
t1."FIRST_NAME" = (SELECT "FIRST_NAME" as "fn" FROM "test_space" WHERE "id" = 1)"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion_t2_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
let mut virtual_t2 = VirtualTable::new();
@@ -442,9 +442,9 @@ fn anonymous_col_index_test() {
WHERE "id" in (SELECT "identification_number" FROM "hash_testing" WHERE "product_units" < 3)
OR "id" in (SELECT "identification_number" FROM "hash_testing" WHERE "product_units" > 5)"#;
- let mut engine = EngineMock::new();
+ let engine = EngineMock::new();
- let mut query = Query::new(&mut engine, sql).unwrap();
+ let mut query = Query::new(&engine, sql).unwrap();
let motion1_id = query.exec_plan.get_ir_plan().get_slices().unwrap()[0][0];
query
.engine
diff --git a/src/parser.rs b/src/parser.rs
index 1b84a86229..944599e716 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -72,7 +72,7 @@ pub extern "C" fn calculate_bucket_id_by_dict(ctx: FunctionCtx, args: FunctionAr
return ret_code;
}
QUERY_ENGINE.with(|e| {
- let engine = &mut *e.borrow_mut();
+ let engine = &*e.borrow();
// Closure for more concise error propagation from calls nested in the bucket calculation
let propagate_err = || -> Result<u64, QueryPlannerError> {
@@ -117,7 +117,7 @@ pub extern "C" fn execute_query(ctx: FunctionCtx, args: FunctionArgs) -> c_int {
return ret_code;
}
QUERY_ENGINE.with(|e| {
- let engine = &mut *e.borrow_mut();
+ let engine = &*e.borrow();
let mut query = match Query::new(engine, args.query.as_str()) {
Ok(q) => q,
Err(e) => {
@@ -162,16 +162,25 @@ fn load_metadata() -> c_int {
}
});
+ let mut is_metadata_empty = false;
+ QUERY_ENGINE.with(|e| {
+ let engine = &*e.borrow();
+ if engine.is_metadata_empty() {
+ is_metadata_empty = true;
+ }
+ });
// Tarantool never yields here, so it is possible to hold
// a mutable reference to the engine.
- if let (Some(schema), Some(timeout)) = (schema, timeout) {
- QUERY_ENGINE.with(|e| {
- let engine = &mut *e.borrow_mut();
- if let Err(e) = engine.update_metadata(schema, timeout) {
- return tarantool::set_error!(TarantoolErrorCode::ProcC, "{}", e.to_string());
- }
- 0
- });
+ if is_metadata_empty {
+ if let (Some(schema), Some(timeout)) = (schema, timeout) {
+ QUERY_ENGINE.with(|e| {
+ let engine = &mut *e.borrow_mut();
+ if let Err(e) = engine.update_metadata(schema, timeout) {
+ return tarantool::set_error!(TarantoolErrorCode::ProcC, "{}", e.to_string());
+ }
+ 0
+ });
+ }
}
0
--
GitLab