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

Storage, Query - deletion bugfix and improvements

Browse Source 
Summary:
Multiple attempts to delete a record dont crash anymore.
Deleting a vertex and its blocking edge in the same delete op now supported.
Utils::Assert - permanent_fail bug fix

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D282
This commit is contained in:
florijan 2017-04-14 15:03:18 +02:00
parent 3d0181b28b
commit 0a09a6ac64
5 changed files with 94 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
src/database/graph_db_accessor.cpp
View File

@ -23,14 +23,14 @@ void GraphDbAccessor::advance_command() {
}
void GraphDbAccessor::commit() {
debug_assert(commited_ == false && aborted_ == false,
debug_assert(!commited_ && !aborted_,
"Already aborted or commited transaction.");
transaction_->commit();
commited_ = true;
}
void GraphDbAccessor::abort() {
debug_assert(commited_ == false && aborted_ == false,
debug_assert(!commited_ && !aborted_,
"Already aborted or commited transaction.");
transaction_->abort();
aborted_ = true;
@ -56,6 +56,10 @@ size_t GraphDbAccessor::vertices_count(const GraphDbTypes::Label &label) {
bool GraphDbAccessor::remove_vertex(VertexAccessor &vertex_accessor) {
vertex_accessor.SwitchNew();
// it's possible the vertex was removed already in this transaction
// due to it getting matched multiple times by some patterns
// we can only delete it once, so check if it's already deleted
if (vertex_accessor.current_->is_deleted_by(*transaction_)) return true;
if (vertex_accessor.out_degree() > 0 || vertex_accessor.in_degree() > 0)
return false;
@ -68,8 +72,8 @@ void GraphDbAccessor::detach_remove_vertex(VertexAccessor &vertex_accessor) {
for (auto edge_accessor : vertex_accessor.in()) remove_edge(edge_accessor);
vertex_accessor.SwitchNew();
for (auto edge_accessor : vertex_accessor.out()) remove_edge(edge_accessor);
vertex_accessor.vlist_->remove(vertex_accessor.SwitchNew().current_,
*transaction_);
if (!remove_vertex(vertex_accessor))
permanent_fail("Unable to remove vertex after all edges detached");
}
EdgeAccessor GraphDbAccessor::insert_edge(VertexAccessor &from,
@ -123,10 +127,14 @@ void swap_out_edge(std::vector<mvcc::VersionList<Edge> *> &edges,
}
void GraphDbAccessor::remove_edge(EdgeAccessor &edge_accessor) {
// it's possible the edge was removed already in this transaction
// due to it getting matched multiple times by some patterns
// we can only delete it once, so check if it's already deleted
edge_accessor.SwitchNew();
if (edge_accessor.current_->is_deleted_by(*transaction_)) return;
swap_out_edge(edge_accessor.from().update().out_, edge_accessor.vlist_);
swap_out_edge(edge_accessor.to().update().in_, edge_accessor.vlist_);
edge_accessor.vlist_->remove(edge_accessor.SwitchNew().current_,
*transaction_);
edge_accessor.vlist_->remove(edge_accessor.current_, *transaction_);
}
GraphDbTypes::Label GraphDbAccessor::label(const std::string &label_name) {

3
src/database/graph_db_accessor.hpp
View File

@ -59,6 +59,9 @@ class GraphDbAccessor {
* or incoming edges, it is not deleted. See `detach_remove_vertex` if you
* want to remove a vertex regardless of connectivity.
*
* If the vertex has already been deleted by the current transaction+command,
* this function will not do anything and will return true.
*
* @param vertex_accessor Accessor to vertex.
* @return If or not the vertex was deleted.
*/

37
src/query/plan/operator.cpp
View File

@ -549,30 +549,43 @@ bool Delete::DeleteCursor::Pull(Frame &frame, const SymbolTable &symbol_table) {
// Delete should get the latest information, this way it is also possible to
// delete newly added nodes and edges.
evaluator.SwitchNew();
// collect expressions results so edges can get deleted before vertices
// this is necessary because an edge that gets deleted could block vertex
// deletion
std::vector<TypedValue> expression_results;
expression_results.reserve(self_.expressions_.size());
for (Expression *expression : self_.expressions_) {
expression->Accept(evaluator);
TypedValue value = evaluator.PopBack();
switch (value.type()) {
case TypedValue::Type::Null:
// if we got a Null, that's OK, probably it's an OPTIONAL MATCH
return true;
case TypedValue::Type::Vertex:
expression_results.emplace_back(evaluator.PopBack());
}
// delete edges first
for (TypedValue &expression_result : expression_results)
if (expression_result.type() == TypedValue::Type::Edge)
db_.remove_edge(expression_result.Value<EdgeAccessor>());
// delete vertices
for (TypedValue &expression_result : expression_results)
switch (expression_result.type()) {
case TypedValue::Type::Vertex: {
VertexAccessor &va = expression_result.Value<VertexAccessor>();
va.SwitchNew(); // necessary because an edge deletion could have
// updated
if (self_.detach_)
db_.detach_remove_vertex(value.Value<VertexAccessor>());
else if (!db_.remove_vertex(value.Value<VertexAccessor>()))
db_.detach_remove_vertex(va);
else if (!db_.remove_vertex(va))
throw query::QueryRuntimeException(
"Failed to remove vertex because of it's existing "
"connections. Consider using DETACH DELETE.");
break;
}
case TypedValue::Type::Edge:
db_.remove_edge(value.Value<EdgeAccessor>());
break;
case TypedValue::Type::Path:
// TODO consider path deletion
// check we're not trying to delete anything except vertices and edges
default:
throw TypedValueException("Can only delete edges and vertices");
}
}
return true;
}

2
src/utils/assert.hpp
View File

@ -41,10 +41,12 @@
}
#define permanent_fail(message) \
{ \
std::ostringstream s; \
s << message; \
__handle_assert_message(s.str()); \
std::exit(EXIT_FAILURE); \
}
/** \
* debug assertion is more like standard C assert but with custom \
* define which controls when the assertion will be active \

50
tests/unit/query_plan_create_set_remove_delete.cpp
View File

@ -324,6 +324,56 @@ TEST(QueryPlan, Delete) {
}
}
TEST(QueryPlan, DeleteTwiceDeleteBlockingEdge) {
// test deleting the same vertex and edge multiple times
//
// also test vertex deletion succeeds if the prohibiting
// edge is deleted in the same logical op
//
// we test both with the following queries (note the
// undirected edge in MATCH):
//
// CREATE ()-[:T]->()
// MATCH (n)-[r]-(m) [DETACH] DELETE n, r, m
auto test_delete = [](bool detach) {
Dbms dbms;
auto dba = dbms.active();
auto v1 = dba->insert_vertex();
auto v2 = dba->insert_vertex();
dba->insert_edge(v1, v2, dba->edge_type("T"));
dba->advance_command();
EXPECT_EQ(2, CountIterable(dba->vertices()));
EXPECT_EQ(1, CountIterable(dba->edges()));
AstTreeStorage storage;
SymbolTable symbol_table;
auto n = MakeScanAll(storage, symbol_table, "n");
auto r_m = MakeExpand(storage, symbol_table, n.op_, n.sym_, "r",
EdgeAtom::Direction::BOTH, false, "m", false);
// getter expressions for deletion
auto n_get = storage.Create<Identifier>("n");
symbol_table[*n_get] = n.sym_;
auto r_get = storage.Create<Identifier>("r");
symbol_table[*r_get] = r_m.edge_sym_;
auto m_get = storage.Create<Identifier>("m");
symbol_table[*m_get] = r_m.node_sym_;
auto delete_op = std::make_shared<plan::Delete>(
r_m.op_, std::vector<Expression *>{n_get, r_get, m_get}, detach);
EXPECT_EQ(2, PullAll(delete_op, *dba, symbol_table));
dba->advance_command();
EXPECT_EQ(0, CountIterable(dba->vertices()));
EXPECT_EQ(0, CountIterable(dba->edges()));
};
test_delete(true);
test_delete(false);
}
TEST(QueryPlan, DeleteReturn) {
Dbms dbms;
auto dba = dbms.active();