From 795ca021a2ffe3ac46a42926dfea6b9b4e4eb9ac Mon Sep 17 00:00:00 2001
From: Denis Smirnov <sd@picodata.io>
Date: Fri, 30 Dec 2022 20:48:28 +0700
Subject: [PATCH] fix: do not cache subplans with virtual tables
We should not use the cache on the storage if the plan contains
virtual tables, as they can contain different amount of tuples that
are not taken into account when calculating the cache key.
Fixes: issue 308.
---
.../src/api/exec_query/protocol.rs | 11 ++-
sbroad-cartridge/src/cartridge/router.rs | 13 +++-
sbroad-cartridge/src/cartridge/storage.rs | 70 ++++++++++---------
3 files changed, 60 insertions(+), 34 deletions(-)
diff --git a/sbroad-cartridge/src/api/exec_query/protocol.rs b/sbroad-cartridge/src/api/exec_query/protocol.rs
index 2f4b2c12ed..1879e3bef6 100644
--- a/sbroad-cartridge/src/api/exec_query/protocol.rs
+++ b/sbroad-cartridge/src/api/exec_query/protocol.rs
@@ -56,6 +56,7 @@ pub struct RequiredData {
pub(crate) plan_id: String,
pub(crate) parameters: Vec<Value>,
pub(crate) query_type: QueryType,
+ pub(crate) can_be_cached: bool,
context: ContextCarrier,
force_trace: bool,
trace_id: Option<String>,
@@ -67,6 +68,7 @@ impl Default for RequiredData {
plan_id: String::new(),
parameters: vec![],
query_type: QueryType::DQL,
+ can_be_cached: true,
context: ContextCarrier::empty(),
force_trace: false,
trace_id: None,
@@ -98,7 +100,12 @@ impl TryFrom<&[u8]> for RequiredData {
}
impl RequiredData {
- pub fn new(plan_id: String, parameters: Vec<Value>, query_type: QueryType) -> Self {
+ pub fn new(
+ plan_id: String,
+ parameters: Vec<Value>,
+ query_type: QueryType,
+ can_be_cached: bool,
+ ) -> Self {
let mut carrier = HashMap::new();
inject_context(&mut carrier);
let force_trace = force_trace();
@@ -107,6 +114,7 @@ impl RequiredData {
plan_id,
parameters,
query_type,
+ can_be_cached,
context: ContextCarrier::empty(),
force_trace,
trace_id: None,
@@ -116,6 +124,7 @@ impl RequiredData {
plan_id,
parameters,
query_type,
+ can_be_cached,
context: ContextCarrier::new(carrier),
force_trace,
trace_id: Some(current_id()),
diff --git a/sbroad-cartridge/src/cartridge/router.rs b/sbroad-cartridge/src/cartridge/router.rs
index 3967d1f8c3..ac19eebdaf 100644
--- a/sbroad-cartridge/src/cartridge/router.rs
+++ b/sbroad-cartridge/src/cartridge/router.rs
@@ -206,6 +206,12 @@ impl Coordinator for RouterRuntime {
}
};
+ // We should not use the cache on the storage if the plan contains virtual tables,
+ // as they can contain different amount of tuples that are not taken into account
+ // when calculating the cache key.
+ let can_be_cached =
+ |plan: &ExecutionPlan| plan.get_vtables().map_or(true, HashMap::is_empty);
+
let encode_plan =
|exec_plan: ExecutionPlan| -> Result<(Binary, Binary), QueryPlannerError> {
let sp_top_id = exec_plan.get_ir_plan().get_top()?;
@@ -215,7 +221,12 @@ impl Coordinator for RouterRuntime {
// Virtual tables in the plan must be already filtered, so we can use all buckets here.
let params = exec_plan.to_params(&nodes, &Buckets::All)?;
let query_type = exec_plan.query_type()?;
- let required_data = RequiredData::new(sub_plan_id.clone(), params, query_type);
+ let required_data = RequiredData::new(
+ sub_plan_id.clone(),
+ params,
+ query_type,
+ can_be_cached(&exec_plan),
+ );
let encoded_required_data = EncodedRequiredData::try_from(required_data)?;
let raw_required_data: Vec<u8> = encoded_required_data.into();
let optional_data = OptionalData::new(exec_plan, ordered);
diff --git a/sbroad-cartridge/src/cartridge/storage.rs b/sbroad-cartridge/src/cartridge/storage.rs
index 8ec861bd93..5664b0dada 100644
--- a/sbroad-cartridge/src/cartridge/storage.rs
+++ b/sbroad-cartridge/src/cartridge/storage.rs
@@ -176,36 +176,40 @@ impl StorageRuntime {
let buckets = Buckets::All;
// Look for the prepared statement in the cache.
- if let Some(stmt) = self
- .cache
- .try_borrow_mut()
- .map_err(|e| QueryPlannerError::CustomError(format!("Failed to borrow cache: {e}")))?
- .get(&plan_id)?
- {
- let stmt_id = stmt.id()?;
- // The statement was found in the cache, so we can execute it.
+ if required.can_be_cached {
+ if let Some(stmt) = self
+ .cache
+ .try_borrow_mut()
+ .map_err(|e| {
+ QueryPlannerError::CustomError(format!("Failed to borrow cache: {e}"))
+ })?
+ .get(&plan_id)?
+ {
+ let stmt_id = stmt.id()?;
+ // The statement was found in the cache, so we can execute it.
+ debug!(
+ Option::from("execute plan"),
+ &format!("Execute prepared statement {stmt:?}"),
+ );
+ let result = match required.query_type {
+ QueryType::DML => write_prepared(stmt_id, "", &required.parameters),
+ QueryType::DQL => read_prepared(stmt_id, "", &required.parameters),
+ };
+
+ // If prepared statement is invalid for some reason, fallback to the long pass
+ // and recompile the query.
+ if result.is_ok() {
+ return result;
+ }
+ }
debug!(
Option::from("execute plan"),
- &format!("Execute prepared statement {stmt:?}"),
+ &format!("Failed to find a plan (id {plan_id}) in the cache."),
);
- let result = match required.query_type {
- QueryType::DML => write_prepared(stmt_id, "", &required.parameters),
- QueryType::DQL => read_prepared(stmt_id, "", &required.parameters),
- };
-
- // If prepared statement is invalid for some reason, fallback to the long pass
- // and recompile the query.
- if result.is_ok() {
- return result;
- }
}
// Find a statement in the Tarantool's cache or prepare it
// (i.e. compile and put into the cache).
- debug!(
- Option::from("execute plan"),
- &format!("Failed to find a plan (id {plan_id}) in the cache."),
- );
let data = std::mem::take(raw_optional);
let mut optional = OptionalData::try_from(EncodedOptionalData::from(data))?;
optional.exec_plan.get_mut_ir_plan().restore_constants()?;
@@ -222,15 +226,17 @@ impl StorageRuntime {
stmt.pattern()?
),
);
- self.cache
- .try_borrow_mut()
- .map_err(|e| {
- QueryPlannerError::CustomError(format!(
- "Failed to put prepared statement {:?} into the cache: {:?}",
- stmt, e
- ))
- })?
- .put(plan_id, stmt)?;
+ if required.can_be_cached {
+ self.cache
+ .try_borrow_mut()
+ .map_err(|e| {
+ QueryPlannerError::CustomError(format!(
+ "Failed to put prepared statement {:?} into the cache: {:?}",
+ stmt, e
+ ))
+ })?
+ .put(plan_id, stmt)?;
+ }
// The statement was found in the cache, so we can execute it.
debug!(
Option::from("execute plan"),
--
GitLab