Handle analytical mode in community_detection during parallel changes #400

Darych · 2023-10-23T17:00:15Z

Description

NB: PR taken over by @antepusic; connected with memgraph/memgraph#1395

Since there are no ACID guarantees in analytical storage mode, the graph may get modified by parallel transactions while the user’s query is running. Query procedures and functions that return graph elements (nodes and relationships) should be robust to the deletion of graph elements, and this PR aims to fix that.

This PR contains changes to query modules that ensure the intended behavior:

Query modules written with the C and C++ APIs check the existence of values that may contain graph elements before returning them (Python API query modules do this under the hood)

Merge commit message:

Add proper handling of deleted return values for query procedures and functions ran in analytical mode

Pull request type

Bugfix

Related issues

Resolves memgraph/memgraph#1036 (together with memgraph/memgraph#1395)

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

Core algorithm/module implementation
Query module implementation
Unit tests
End-to-end tests
Code documentation
README short description
Documentation on memgraph/docs
Update GQLALchemy signatures in query builder using query module signature generator

######################################

antoniofilipovic · 2023-10-27T15:13:49Z

cpp/mg_utility/mg_graph.hpp

+    if (memgraph_to_inner_id_.find(memgraph_id) == memgraph_to_inner_id_.end()) {
+      if (IsTransactional()) {
+        throw mg_exception::InvalidIDException();
+      }
+      return std::nullopt;
+    }
+    return memgraph_to_inner_id_.at(memgraph_id);


You can keep a reference to iterator here, and instead of doing find, and then at, you can do find, and return element if it is there

as51340

I think good job in general, take care about duplication please :)

as51340 · 2023-11-20T07:11:38Z

cpp/community_detection_module/community_detection_online_module.cpp

+
+        // Otherwise:
+        auto record = record_factory.NewRecord();
+        record.Insert(kFieldNode.data(), graph.GetNodeById(mgp::Id::FromUint(node_id)));


Cache result of calling graph.GetNodeById, no need to call it twice

as51340 · 2023-11-20T07:11:47Z

cpp/community_detection_module/community_detection_online_module.cpp

+
+          // Otherwise:
+          auto record = record_factory.NewRecord();
+          record.Insert(kFieldNode.data(), graph.GetNodeById(mgp::Id::FromUint(node_id)));


as51340 · 2023-11-20T07:12:25Z

cpp/community_detection_module/community_detection_online_module.cpp

+        // As IN_MEMORY_ANALYTICAL doesn’t offer ACID guarantees, check if the graph elements in the result exist
+        try {
+          // If so, throw an exception:
+          const auto maybe_node = graph.GetNodeById(mgp::Id::FromUint(node_id));


What exceptions can be thrown in GetNodeById?

NotFoundException
https://github.com/memgraph/memgraph/blob/master/include/mgp.hpp#L1959

cpp/mg_utility/data_structures/graph_view.hpp

as51340 · 2023-11-20T07:16:57Z

cpp/mg_utility/mg_utils.hpp

+  bool isTransactionalStorage = mgp::graph_is_transactional(memgraph_graph);
+  graph->SetIsTransactional(isTransactionalStorage);


Suggested change

bool isTransactionalStorage = mgp::graph_is_transactional(memgraph_graph);

graph->SetIsTransactional(isTransactionalStorage);

graph->SetIsTransactional(mgp::graph_is_transactional(memgraph_graph));

as51340 · 2023-11-20T07:17:47Z

cpp/pagerank_module/pagerank_module.cpp

@@ -1,4 +1,6 @@
+#include <chrono>


Why these 2 things got included?

Removed – I’d originally added them to help with replicating the issue.

as51340 · 2023-11-20T07:18:53Z

cpp/betweenness_centrality_module/betweenness_centrality_module.cpp

+  auto *vertex = mgp::graph_get_vertex_by_id(graph, mgp_vertex_id{.as_int = static_cast<int64_t>(node_id)}, memory);
+  if (!vertex) {
+    if (mgp::graph_is_transactional(graph)) {
+      throw mg_exception::InvalidIDException();
+    }
+    // In IN_MEMORY_ANALYTICAL mode, vertices/edges may be erased by parallel transactions.
+    return;
+  }
+


These code is duplicates over all modules... It would be good to respect to respect DRY principle here and abstract this code into a method.

as51340 · 2023-11-20T07:23:35Z

cpp/mg_utility/mg_graph.hpp

+    if (memgraph_to_inner_id_.find(memgraph_id) == memgraph_to_inner_id_.end()) {
+      if (IsTransactional()) {
+        throw mg_exception::InvalidIDException();
+      }
+      return std::nullopt;
+    }
+    return memgraph_to_inner_id_.at(memgraph_id);


Suggested change

if (memgraph_to_inner_id_.find(memgraph_id) == memgraph_to_inner_id_.end()) {

if (IsTransactional()) {

throw mg_exception::InvalidIDException();

}

return std::nullopt;

}

return memgraph_to_inner_id_.at(memgraph_id);

auto it = memgraph_to_inner_id_.find(memgraph_id);

if (it != memgraph_to_inner_id_.end()) {

return *it;

}

if (IsTransactional()) {

throw mg_exception::InvalidIDException();

}

return std::nullopt;

}

as51340 · 2023-11-20T07:27:22Z

And you have build errors, please take care about that before requesting review again. For next time tip: it is better to request review after build errors are fixed since it gives reviewer more confidence that hey this is good and meaningful fix without negative impact on the rest of the system

Darych · 2023-11-23T17:27:40Z

cpp/katz_centrality_module/katz_centrality_online_module.cpp

+
+          // Otherwise:
+          auto record = record_factory.NewRecord();
+          record.Insert(kFieldNode, graph.GetNodeById(mgp::Id::FromUint(node_id)));


Instead of calling graph.GetNodeById once again, can we use maybe_node here?

Darych · 2023-11-23T17:30:09Z

cpp/katz_centrality_module/katz_centrality_online_module.cpp

+        // As IN_MEMORY_ANALYTICAL doesn’t offer ACID guarantees, check if the graph elements in the result exist
+        try {
+          // If so, throw an exception:
+          const auto maybe_node = graph.GetNodeById(mgp::Id::FromUint(node_id));


Since the function returns not optional Node, IMO it's better to name the variable as node and reuse it later.

Darych · 2023-11-23T17:32:18Z

cpp/pagerank_module/pagerank_online_module.cpp

+      // As IN_MEMORY_ANALYTICAL doesn’t offer ACID guarantees, check if the graph elements in the result exist
+      try {
+        // If so, throw an exception:
+        const auto maybe_node = graph.GetNodeById(mgp::Id::FromUint(node_id));


The same comment as in katz_centrality_online_module.cpp

Darych · 2023-11-23T17:37:21Z

cpp/community_detection_module/community_detection_online_module.cpp

+        // As IN_MEMORY_ANALYTICAL doesn’t offer ACID guarantees, check if the graph elements in the result exist
+        try {
+          // If so, throw an exception:
+          const auto maybe_node = graph.GetNodeById(mgp::Id::FromUint(node_id));


NotFoundException
https://github.com/memgraph/memgraph/blob/master/include/mgp.hpp#L1959

cpp/mg_utility/data_structures/graph_view.hpp

…mage into handle-analytical-mode

Darych · 2023-11-28T09:28:59Z

My initial idea was to make body of GetInnerNodeIdOpt a default implementation for GetInnerNodeId, so we will have just one implementation. I mentioned about this HERE
Currently. we have 2 implementations and users should think which one they need to use in their modules.
@antoniofilipovic what do you think?

Darych

LGTM. I can't approve the PR since I'm author.

…mage into handle-analytical-mode

sonarcloud · 2023-12-05T18:45:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.7% Duplication

.github/workflows/test.yml

antepusic · 2023-12-07T11:41:27Z

@vpavicic As this PR is the MAGE side of memgraph/memgraph#1395, they are covered by the same docs PR (memgraph/documentation#361) and should both be linked in the same release note.

Handle analytical mode in community_detection during parallel changes

323cec8

gitbuda assigned Darych Oct 24, 2023

gitbuda linked an issue Oct 24, 2023 that may be closed by this pull request

Fix running mage modules/algorithms under IN_MEMORY_ANALYTICAL mode and streaming memgraph/memgraph#1036

Closed

Darych added 2 commits October 24, 2023 09:42

Fix core dump when vertex is already deleted from memgraph graph

8b9d4e0

First create for vertex existence and insert if exists

fa23b72

antoniofilipovic reviewed Oct 27, 2023

View reviewed changes

Darych mentioned this pull request Oct 30, 2023

Fix running mage modules/algorithms under IN_MEMORY_ANALYTICAL mode and streaming memgraph/memgraph#1036

Closed

antepusic assigned antepusic and unassigned Darych Oct 31, 2023

Add fixes for the C and C++ APIs

0805098

antepusic added Needs new memgraph lang: cpp Issue on C++ codebase In progress Docs needed Docs needed and removed lang: cpp Issue on C++ codebase labels Nov 6, 2023

antepusic added 4 commits November 9, 2023 01:31

Fix doubled ; after statement

c314170

Improve the C and C++ API fixes

4c8f555

Fix C API result returning

f404305

Add remaining deletion checks in the C++ API

57e5909

antepusic added status: ready PR is ready for review and removed In progress labels Nov 14, 2023

antepusic requested review from Josipmrden and antoniofilipovic November 14, 2023 01:06

antepusic mentioned this pull request Nov 14, 2023

Handle analytical mode in community_detection during parallel changes memgraph/memgraph#1395

Merged

4 tasks

antepusic marked this pull request as ready for review November 15, 2023 08:38

antepusic added 2 commits November 15, 2023 10:00

Resolve merge conflict

5d93ba3

Merge branch 'main' into handle-analytical-mode

4d4727c

antepusic requested a review from as51340 November 17, 2023 13:22

as51340 requested changes Nov 20, 2023

View reviewed changes

Darych commented Nov 23, 2023

View reviewed changes

antepusic added 5 commits November 25, 2023 17:13

Add review suggestions

507aec0

Merge branch 'main' into handle-analytical-mode

baac657

Merge branch 'handle-analytical-mode' of https://github.com/memgraph/…

710dac5

…mage into handle-analytical-mode

Add review comments

a8106dc

Fix build

23cb924

antepusic modified the milestones: 1.12.0, 1.13.0 Nov 28, 2023

Darych commented Nov 29, 2023

View reviewed changes

antepusic mentioned this pull request Dec 4, 2023

Fix refactor rvalue bind #427

Merged

16 tasks

antepusic added 8 commits December 5, 2023 14:54

Merge branch 'main' into handle-analytical-mode

865f6be

Get checks to use latest memgraph

c67d161

Merge branch 'handle-analytical-mode' of https://github.com/memgraph/…

86589fb

…mage into handle-analytical-mode

Fix version

05e08b9

Update submodule to current master

bc70a80

Fix node existence check

508425a

Merge branch 'main' into handle-analytical-mode

a2e6b3c

Extract duplicate code into a method

e8180fc

antepusic requested a review from as51340 December 5, 2023 18:28

Remove duplication in community_detection_online

1cd20ac

antepusic removed the request for review from Josipmrden December 5, 2023 22:36

as51340 approved these changes Dec 7, 2023

View reviewed changes

.github/workflows/test.yml Show resolved Hide resolved

antepusic mentioned this pull request Dec 7, 2023

[memgraph-v2-13 < 361] Document query module behavior in analytical mode memgraph/documentation#361

Merged

9 tasks

antepusic removed the Needs new memgraph label Dec 7, 2023

antepusic merged commit 085bd27 into main Dec 7, 2023
6 checks passed

antepusic deleted the handle-analytical-mode branch December 7, 2023 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle analytical mode in community_detection during parallel changes #400

Handle analytical mode in community_detection during parallel changes #400

Darych commented Oct 23, 2023 •

edited by antepusic

antoniofilipovic Oct 27, 2023

antepusic Nov 25, 2023

as51340 left a comment

as51340 Nov 20, 2023

as51340 Nov 20, 2023

as51340 Nov 20, 2023

Darych Nov 23, 2023

as51340 Nov 20, 2023

antepusic Nov 27, 2023

as51340 Nov 20, 2023

antepusic Nov 20, 2023 •

edited

as51340 Nov 20, 2023

antepusic Dec 5, 2023

as51340 Nov 20, 2023

antepusic Nov 27, 2023

as51340 commented Nov 20, 2023

Darych Nov 23, 2023

antepusic Nov 27, 2023

Darych Nov 23, 2023

antepusic Nov 27, 2023

Darych Nov 23, 2023

antepusic Nov 27, 2023

Darych Nov 23, 2023

Darych commented Nov 28, 2023

Darych left a comment

sonarcloud bot commented Dec 5, 2023

antepusic commented Dec 7, 2023

		bool isTransactionalStorage = mgp::graph_is_transactional(memgraph_graph);
		graph->SetIsTransactional(isTransactionalStorage);

	bool isTransactionalStorage = mgp::graph_is_transactional(memgraph_graph);
	graph->SetIsTransactional(isTransactionalStorage);
	graph->SetIsTransactional(mgp::graph_is_transactional(memgraph_graph));

Handle analytical mode in community_detection during parallel changes #400

Handle analytical mode in community_detection during parallel changes #400

Conversation

Darych commented Oct 23, 2023 • edited by antepusic

Description

Pull request type

Related issues

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

Choose a reason for hiding this comment

Choose a reason for hiding this comment

as51340 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antepusic Nov 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

as51340 commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darych commented Nov 28, 2023

Darych left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 5, 2023

antepusic commented Dec 7, 2023

Darych commented Oct 23, 2023 •

edited by antepusic

antepusic Nov 20, 2023 •

edited