From b6067673bab0c258b5e391c17014fde90d10f20c Mon Sep 17 00:00:00 2001
From: Denis Smirnov <sd@picodata.io>
Date: Thu, 11 Aug 2022 12:11:29 +0700
Subject: [PATCH] feat: never panic while borrowing the cartridge cache
---
src/api/calculate_bucket_id.rs | 17 ++++++---
src/api/calculate_bucket_id_by_dict.rs | 15 +++++---
src/api/exec_query.rs | 30 ++++++++++++----
src/api/explain.rs | 15 ++++++--
src/api/helper.rs | 46 ++++++++++++++++++------
src/api/invalidate_cached_schema.rs | 30 ++++++----------
src/executor.rs | 9 +++--
src/executor/engine/cartridge/router.rs | 4 ++-
src/executor/engine/cartridge/storage.rs | 10 +++++-
9 files changed, 124 insertions(+), 52 deletions(-)
diff --git a/src/api/calculate_bucket_id.rs b/src/api/calculate_bucket_id.rs
index a0aa22104d..16622b9124 100644
--- a/src/api/calculate_bucket_id.rs
+++ b/src/api/calculate_bucket_id.rs
@@ -34,11 +34,18 @@ pub extern "C" fn calculate_bucket_id(ctx: FunctionCtx, args: FunctionArgs) -> c
Err(e) => return tarantool::set_error!(TarantoolErrorCode::ProcC, "{:?}", e),
};
- COORDINATOR_ENGINE.with(|e| {
- let engine = &mut *e.borrow_mut();
-
- let result = engine.determine_bucket_id(&[&bucket_str]);
-
+ COORDINATOR_ENGINE.with(|engine| {
+ let runtime = match engine.try_borrow() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to borrow the runtime while calculating a bucket id: {:?}",
+ e
+ )
+ }
+ };
+ let result = runtime.determine_bucket_id(&[&bucket_str]);
ctx.return_mp(&result).unwrap();
0
})
diff --git a/src/api/calculate_bucket_id_by_dict.rs b/src/api/calculate_bucket_id_by_dict.rs
index 3e662a9437..f7d6cdc0ff 100644
--- a/src/api/calculate_bucket_id_by_dict.rs
+++ b/src/api/calculate_bucket_id_by_dict.rs
@@ -61,12 +61,19 @@ pub extern "C" fn calculate_bucket_id_by_dict(ctx: FunctionCtx, args: FunctionAr
if ret_code != 0 {
return ret_code;
}
- COORDINATOR_ENGINE.with(|e| {
- let engine = &*e.borrow();
+ COORDINATOR_ENGINE.with(|engine| {
+ let runtime = match engine.try_borrow() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(TarantoolErrorCode::ProcC,
+ "Failed to borrow the runtime while calculating a bucket id by dictionary: {:?}",
+ e.to_string());
+ }
+ };
- match engine.extract_sharding_keys(params.space, ¶ms.rec) {
+ match runtime.extract_sharding_keys(params.space, ¶ms.rec) {
Ok(tuple) => {
- let bucket_id = engine.determine_bucket_id(&tuple);
+ let bucket_id = runtime.determine_bucket_id(&tuple);
ctx.return_mp(&bucket_id).unwrap();
0
}
diff --git a/src/api/exec_query.rs b/src/api/exec_query.rs
index 9bf5ef3a41..c4e17eb8e9 100644
--- a/src/api/exec_query.rs
+++ b/src/api/exec_query.rs
@@ -23,9 +23,18 @@ pub extern "C" fn dispatch_query(ctx: FunctionCtx, args: FunctionArgs) -> c_int
if ret_code != 0 {
return ret_code;
}
- COORDINATOR_ENGINE.with(|e| {
- let engine = &*e.borrow();
- let mut query = match Query::new(engine, &lua_params.pattern, &lua_params.params) {
+ COORDINATOR_ENGINE.with(|engine| {
+ let runtime = match engine.try_borrow() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to borrow the runtime while dispatching the query: {}",
+ e.to_string()
+ );
+ }
+ };
+ let mut query = match Query::new(&*runtime, &lua_params.pattern, &lua_params.params) {
Ok(q) => q,
Err(e) => {
say(
@@ -104,9 +113,18 @@ pub extern "C" fn execute_query(ctx: FunctionCtx, args: FunctionArgs) -> c_int {
if ret_code != 0 {
return ret_code;
}
- SEGMENT_ENGINE.with(|e| {
- let engine = &*e.borrow();
- match engine.execute(
+ SEGMENT_ENGINE.with(|engine| {
+ let runtime = match engine.try_borrow() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to borrow the runtime while executing the query: {}",
+ e.to_string()
+ );
+ }
+ };
+ match runtime.execute(
lua_params.pattern.as_str(),
&lua_params.params,
lua_params.is_data_modifier,
diff --git a/src/api/explain.rs b/src/api/explain.rs
index dbcecde54b..15d38322d8 100644
--- a/src/api/explain.rs
+++ b/src/api/explain.rs
@@ -39,9 +39,18 @@ pub extern "C" fn explain(ctx: FunctionCtx, args: FunctionArgs) -> c_int {
if ret_code != 0 {
return ret_code;
}
- COORDINATOR_ENGINE.with(|e| {
- let engine = &*e.borrow();
- let query = match Query::new(engine, &lua_params.query, &[]) {
+ COORDINATOR_ENGINE.with(|engine| {
+ let runtime = match engine.try_borrow() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to borrow runtime while explaining the query: {}",
+ e.to_string()
+ );
+ }
+ };
+ let query = match Query::new(&*runtime, &lua_params.query, &[]) {
Ok(q) => q,
Err(e) => {
say(
diff --git a/src/api/helper.rs b/src/api/helper.rs
index 489efe47eb..f05716f195 100644
--- a/src/api/helper.rs
+++ b/src/api/helper.rs
@@ -4,28 +4,54 @@ use std::os::raw::c_int;
use std::thread::LocalKey;
use tarantool::error::TarantoolErrorCode;
-pub fn load_config<Runtime>(runtime: &'static LocalKey<RefCell<Runtime>>) -> c_int
+pub fn load_config<Runtime>(engine: &'static LocalKey<RefCell<Runtime>>) -> c_int
where
Runtime: Configuration,
{
// Tarantool can yield in the middle of a current closure,
// so we can hold only an immutable reference to the engine.
let mut config: Option<_> = None;
- (*runtime).with(|engine| match engine.borrow().get_config() {
- Ok(conf) => {
- config = conf;
- 0
- }
- Err(e) => {
- return tarantool::set_error!(TarantoolErrorCode::ProcC, "{}", e.to_string());
+ (*engine).with(|engine| {
+ let runtime = match engine.try_borrow() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to borrow the runtime while loading configuration: {}",
+ e.to_string()
+ );
+ }
+ };
+ match runtime.get_config() {
+ Ok(conf) => {
+ config = conf;
+ 0
+ }
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to get configuration: {}",
+ e.to_string()
+ );
+ }
}
});
// Tarantool never yields here, so it is possible to hold
// a mutable reference to the engine.
if let Some(config) = config {
- (*runtime).with(|engine| {
- engine.borrow_mut().update_config(config);
+ (*engine).with(|runtime| {
+ let mut runtime = match runtime.try_borrow_mut() {
+ Ok(runtime) => runtime,
+ Err(e) => {
+ return tarantool::set_error!(
+ TarantoolErrorCode::ProcC,
+ "Failed to borrow the runtime while updating configuration: {}",
+ e.to_string()
+ );
+ }
+ };
+ runtime.update_config(config);
0
});
}
diff --git a/src/api/invalidate_cached_schema.rs b/src/api/invalidate_cached_schema.rs
index d1295ba51f..0a2bad396d 100644
--- a/src/api/invalidate_cached_schema.rs
+++ b/src/api/invalidate_cached_schema.rs
@@ -2,41 +2,33 @@ use std::cell::RefCell;
use std::os::raw::c_int;
use std::thread::LocalKey;
use tarantool::error::TarantoolErrorCode;
-use tarantool::log::{say, SayLevel};
use tarantool::tuple::{FunctionArgs, FunctionCtx};
use crate::api::{COORDINATOR_ENGINE, SEGMENT_ENGINE};
use crate::executor::engine::Configuration;
fn clear_cached_config<Runtime>(
- runtime: &'static LocalKey<RefCell<Runtime>>,
+ engine: &'static LocalKey<RefCell<Runtime>>,
ctx: &FunctionCtx,
_: FunctionArgs,
) -> c_int
where
Runtime: Configuration,
{
- runtime.with(|s| {
- if let Ok(mut runtime) = s.try_borrow_mut() {
+ engine.with(|runtime| match runtime.try_borrow_mut() {
+ Ok(mut runtime) => {
runtime.clear_config();
- } else {
- say(
- SayLevel::Error,
- file!(),
- line!().try_into().unwrap_or(0),
- None,
- &format!("{:?}", "Failed to borrow runtime: already in use"),
- );
- tarantool::set_error!(
+ ctx.return_mp(&true).unwrap();
+ 0
+ }
+ Err(e) => {
+ return tarantool::set_error!(
TarantoolErrorCode::ProcC,
- "{}",
- format!("{:?}", "Failed to borrow runtime: already in use")
+ "Failed to borrow the runtime while clearing cached configuration: {}",
+ e.to_string()
);
}
- });
-
- ctx.return_mp(&true).unwrap();
- 0
+ })
}
/// Flush cached configuration in the Rust memory of the coordinator runtime.
diff --git a/src/executor.rs b/src/executor.rs
index 8896eb34ff..bfc38102e6 100644
--- a/src/executor.rs
+++ b/src/executor.rs
@@ -100,13 +100,16 @@ where
let ir_cache = coordinator.ir_cache();
let mut plan = Plan::new();
- if let Some(cached_ir) = ir_cache.borrow_mut().get(&key)? {
- plan = cached_ir.clone();
+ let mut cache = ir_cache.try_borrow_mut().map_err(|e| {
+ QueryPlannerError::CustomError(format!("Failed to create a new query: {:?}", e))
+ })?;
+ if let Some(cached_plan) = cache.get(&key)? {
+ plan = cached_plan.clone();
}
if plan.is_empty() {
let ast = C::ParseTree::new(sql)?;
plan = ast.resolve_metadata(coordinator.cached_config())?;
- ir_cache.borrow_mut().put(key, plan.clone())?;
+ cache.put(key, plan.clone())?;
}
plan.bind_params(params)?;
plan.optimize()?;
diff --git a/src/executor/engine/cartridge/router.rs b/src/executor/engine/cartridge/router.rs
index acd5e28f5e..6feda96c12 100644
--- a/src/executor/engine/cartridge/router.rs
+++ b/src/executor/engine/cartridge/router.rs
@@ -147,7 +147,9 @@ impl Coordinator for RouterRuntime {
type Cache = LRUCache<String, Plan>;
fn clear_ir_cache(&self, capacity: usize) -> Result<(), QueryPlannerError> {
- *self.ir_cache.borrow_mut() = Self::Cache::new(capacity, None)?;
+ *self.ir_cache.try_borrow_mut().map_err(|e| {
+ QueryPlannerError::CustomError(format!("Failed to clear the cache: {:?}", e))
+ })? = Self::Cache::new(capacity, None)?;
Ok(())
}
diff --git a/src/executor/engine/cartridge/storage.rs b/src/executor/engine/cartridge/storage.rs
index 08b0c0a6ed..39f9473144 100644
--- a/src/executor/engine/cartridge/storage.rs
+++ b/src/executor/engine/cartridge/storage.rs
@@ -160,7 +160,15 @@ impl StorageRuntime {
Option::from("execute"),
&format!("Created prepared statement {}", stmt_id),
);
- self.cache.borrow_mut().put(stmt_id, stmt)?;
+ self.cache
+ .try_borrow_mut()
+ .map_err(|e| {
+ QueryPlannerError::CustomError(format!(
+ "Failed to put prepared statement {:?} into the cache: {:?}",
+ stmt, e
+ ))
+ })?
+ .put(stmt_id, stmt)?;
stmt_id
}
Err(e) => {
--
GitLab