tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix interpreter plan ownership

Browse Source 
Summary:
If there was no plan caching, the CachedPlan would not survive
`Interpreter::operator()`, as it was not owned by the
`Interpreter::Result`. If there was caching, it could hapen that the
cache got invalidated while that plan was being interpreted (by
another thread) without that interpretation retaining ownership.

Also simplified code around this.

Reviewers: mislav.bradac, teon.banek

Reviewed By: mislav.bradac

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1084
This commit is contained in:
florijan 2017-12-27 12:11:20 +01:00
parent 8526e79619
commit 7b3d298741
2 changed files with 24 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
src/query/interpreter.cpp
View File

@ -44,27 +44,27 @@ Interpreter::Results Interpreter::operator()(
}
// Check if we have a cached logical plan ready, so that we can skip the
// whole query -> AST -> logical_plan process.
auto cached_plan = [&]() -> std::shared_ptr<CachedPlan> {
// whole query -> AST -> logical_plan process. Note that this local shared_ptr
// might be the only owner of the CachedPlan (if caching is turned off).
// Ensure it lives during the whole interpretation.
std::shared_ptr<CachedPlan> plan(nullptr);
{
auto plan_cache_accessor = plan_cache_.access();
auto plan_cache_it = plan_cache_accessor.find(stripped.hash());
if (plan_cache_it != plan_cache_accessor.end() &&
plan_cache_it->second->IsExpired()) {
// Remove the expired plan.
plan_cache_accessor.remove(stripped.hash());
plan_cache_it = plan_cache_accessor.end();
}
if (plan_cache_it != plan_cache_accessor.end()) {
return plan_cache_it->second;
if (plan_cache_it->second->IsExpired()) {
plan_cache_accessor.remove(stripped.hash());
} else {
plan = plan_cache_it->second;
}
}
return nullptr;
}();
}
auto frontend_time = frontend_timer.Elapsed();
utils::Timer planning_timer;
if (!cached_plan) {
if (!plan) {
AstTreeStorage ast_storage = QueryToAst(stripped, ctx);
SymbolGenerator symbol_generator(ctx.symbol_table_);
ast_storage.query()->Accept(symbol_generator);
@ -74,19 +74,16 @@ Interpreter::Results Interpreter::operator()(
std::tie(tmp_logical_plan, query_plan_cost_estimation) =
MakeLogicalPlan(ast_storage, db_accessor, ctx);
cached_plan = std::make_shared<CachedPlan>(
plan = std::make_shared<CachedPlan>(
std::move(tmp_logical_plan), query_plan_cost_estimation,
ctx.symbol_table_, std::move(ast_storage));
if (FLAGS_query_plan_cache) {
// Cache the generated plan.
auto plan_cache_accessor = plan_cache_.access();
auto plan_cache_it =
plan_cache_accessor.insert(stripped.hash(), cached_plan).first;
cached_plan = plan_cache_it->second;
plan_cache_.access().insert(stripped.hash(), plan);
}
}
auto *logical_plan = &cached_plan->plan();
auto *logical_plan = &plan->plan();
// TODO review: is the check below necessary?
DCHECK(dynamic_cast<const plan::Produce *>(logical_plan) ||
dynamic_cast<const plan::Skip *>(logical_plan) ||
@ -106,14 +103,14 @@ Interpreter::Results Interpreter::operator()(
dynamic_cast<const plan::CreateIndex *>(logical_plan))
<< "Unknown top level LogicalOperator";
ctx.symbol_table_ = cached_plan->symbol_table();
ctx.symbol_table_ = plan->symbol_table();
auto planning_time = planning_timer.Elapsed();
std::map<std::string, TypedValue> summary;
summary["parsing_time"] = frontend_time.count();
summary["planning_time"] = planning_time.count();
summary["cost_estimate"] = cached_plan->cost();
summary["cost_estimate"] = plan->cost();
// TODO: set summary['type'] based on transaction metadata
// the type can't be determined based only on top level LogicalOp
// (for example MATCH DELETE RETURN will have Produce as it's top)
@ -134,8 +131,8 @@ Interpreter::Results Interpreter::operator()(
.first);
}
return Results(std::move(ctx), std::move(cursor), output_symbols, header,
summary, plan_cache_);
return Results(std::move(ctx), plan, std::move(cursor), output_symbols,
header, summary, plan_cache_);
}
AstTreeStorage Interpreter::QueryToAst(const StrippedQuery &stripped,

5
src/query/interpreter.hpp
View File

@ -56,11 +56,13 @@ class Interpreter {
*/
class Results {
friend Interpreter;
Results(Context ctx, std::unique_ptr<query::plan::Cursor> cursor,
Results(Context ctx, std::shared_ptr<CachedPlan> plan,
std::unique_ptr<query::plan::Cursor> cursor,
std::vector<Symbol> output_symbols, std::vector<std::string> header,
std::map<std::string, TypedValue> summary,
ConcurrentMap<HashType, std::shared_ptr<CachedPlan>> &plan_cache)
: ctx_(std::move(ctx)),
plan_(plan),
cursor_(std::move(cursor)),
frame_(ctx_.symbol_table_.max_position()),
output_symbols_(output_symbols),
@ -128,6 +130,7 @@ class Interpreter {
private:
Context ctx_;
std::shared_ptr<CachedPlan> plan_;
std::unique_ptr<query::plan::Cursor> cursor_;
Frame frame_;
std::vector<Symbol> output_symbols_;