Add experimental/v1 ON_DISK_TRANSACTIONAL storage #850

gitbuda · 2023-03-29T20:47:06Z

Closes #842

Add subqueries Co-authored-by: Bruno Sačarić <bruno.sacaric@gmail.com>

andrejtonev · 2023-08-07T13:36:09Z

This merge has dropped the performance on certain benchmarks by some 10% (check single_vertex_read, single_edge_read, etc.)
Double check if this is due to a fundamental memgraph change or something to do with the benchmark itself.

andrejtonev

Just a few comments from today

src/storage/v2/disk/storage.cpp

src/mg_import_csv.cpp

src/query/interpreter.cpp

andrejtonev · 2023-08-07T15:10:03Z

src/storage/v2/storage.cpp

- return replica_info;
- });
-}
+StorageMode Storage::GetStorageMode() const { return storage_mode_; }


Is storage_mode_ protected?
Seems like we are not using the accessor when reading it. Can this cause a race condition?
*similar story for the isolation_level_

for both operations you take exclusive lock (main_lock_)

You take the main_lock_ on write/set but on on read/get.
This is why I was asking about the Accessor. There we take a shared lock on main_lock_.
We shouldn't use the Storage::GetStorageMode() directly since it is not protected. We should access it through the Accessor or add a shared_lock before.
After our discussion on slack, I think we should add GetStorageMode and GetIsolationLevel to the Accessor and DbAccessor and replace all interpreter_context->db->GetStorageMode() calls with a call through an accessor.

yes it could be, that code exists for a long time it is very possible something is broken

why not taking shared_lock in storage class then, why through accessor? Maybe I am missing something but I find it stupid to duplicate all functionalites to accessor level. Especially when main_lock_ is part of the storage

We could take the shared_lock, but that is also currently missing from the get functions. I think...
Also, from the comments on slack, I think we are trying to limit access to the database from the queries to the appropriate accessors and not directly.

andrejtonev

Got up to storage

src/query/interpreter.cpp

andrejtonev · 2023-08-08T09:53:58Z

src/query/interpreter.cpp

+ }
+}
+
+InterpreterContext::InterpreterContext(std::unique_ptr<storage::Storage> db, InterpreterConfig interpreter_config,


Can we use std::unique_ptr<storage::Storage> &&db so the user know they are giving up control of the pointer.

src/query/interpreter.cpp

andrejtonev · 2023-08-08T10:00:52Z

src/query/interpreter.cpp

+ "query. ");
+ }
+
+ main_guard.unlock();


Shouldn't the lock protect until the end?
The lock is part of the storage, so that might cause some problems. But here nothing stops some other thread from accessing the database while it is being switched.

The interpreters lock could work, but it should be held till the end.

lock must be released before new DiskStorage has been created. And yes you are right that some thread can access the DB while being switched. However, this is solved by having creation_storage_mode_ in the accessor. Then upon commit, I just check whether the storage mode is the same as during the start

But what happens to the storage if someone else is using it while you are switching it?
The interpreter_context->db = std::make_unique<memgraph::storage::DiskStorage>(db_config); will destroy the old storage. We have to make sure no one else can possibly be using it while this is happening.

Nothing is stopping someone from accessing the original storage between lines:
if (interpreter_context->interpreters->size() > 1) {
and
interpreter_context->db = std::make_unique<memgraph::storage::DiskStorage>(db_config);.
I imagine that the DiskStorage creation can take some time, so it's not impossible that this happens.
If the only check is on commit, then someone could be using the destroyed storage for a long time.

we don't know what happens, probably it would but the pure point of switching to disk is that you should it alone, at the beginning. So yes there is probably a moment in which something would crash but I wouldn't say the problem is someone is using destroyed storage before commit. But again I agree this should be somehow refactored but this is not easy part. Maybe when we tackle the issue of switching from Lab

Maybe something like this could work.
Every new connection adds an interpreter to the list. So this way they can't use it until we moved the storage.
NOTE: No idea if this code is actually okay.

interpreter_context->interpreters.WithLock([&](const auto &interpreters_) { if (interpreters_.size() > 1) { throw utils::BasicException( "You cannot switch from an in-memory storage mode to the on-disk storage mode when there are " "multiple sessions active. Close all other sessions and try again. As Memgraph Lab uses " "multiple sessions to run queries in parallel, " "it is currently impossible to switch to the on-disk storage mode within Lab. " "Close it, connect to the instance with mgconsole " "and change the storage mode to on-disk from there. Then, you can reconnect with the Lab " "and continue to use the instance as usual."); } main_guard.unlock(); interpreter_context->db = std::make_unique<memgraph::storage::DiskStorage>(interpreter_context->db->config_); });

andrejtonev · 2023-08-08T10:04:31Z

src/query/interpreter.cpp

+ requested_mode == storage::StorageMode::ON_DISK_TRANSACTIONAL) {
+ callback = SwitchMemoryDevice(current_mode, requested_mode, interpreter_context).fn;
+ } else {
+ if (ActiveTransactionsExist(interpreter_context)) {


Shouldn't ActiveTransactionsExist check be done for the on-disk case as well?

yes but for on-disk there is even stronger check: whether there is only one interpreter. I will rework this at some point because of Lab

andrejtonev · 2023-08-08T11:19:43Z

src/query/interpreter.cpp

 if (!db_accessor_) return;

 db_accessor_->Abort();
+ for (auto &qe : query_executions_) {


Could we just call query_executions_.clear()?

cc @Darych but I think no because we support prepared queries or something like this so we have to use it or something. There is a good reason for that I am sure.

andrejtonev · 2023-08-08T11:21:15Z

src/query/interpreter.cpp

+ auto creation_mode = db_accessor_->GetCreationStorageMode();
+ if (creation_mode != storage::StorageMode::ON_DISK_TRANSACTIONAL &&
+ current_storage_mode == storage::StorageMode::ON_DISK_TRANSACTIONAL) {
+ throw QueryException(


Switching to on-disk during a transaction should be impossible.

but I have a check:

if (in_explicit_transaction) { throw StorageModeModificationInMulticommandTxException(); }

I think this check should be unnecessary since the whole architecture should prevent this from happening.
Currently it is needed, but we should rework the code so that this cannot happen and so the check would not be needed.

How would architecture prevent? E.g
BEGIN;
CREATE VERTEX
STORAGE MODE ON_DISK_TRANSACTIONAL; // this will fail now
COMMIT

The void Interpreter::Commit() is called on COMMIT, right?
The STORAGE MODE ON_DISK_TRANSACTIONAL; will fail because we are in a transaction.
So when is this check in Commit() true?

which check?

if (creation_mode != storage::StorageMode::ON_DISK_TRANSACTIONAL && current_storage_mode == storage::StorageMode::ON_DISK_TRANSACTIONAL) {

That check is done in the Commit() and should never be true, right?
Even if it could be true in the current implementation, I think we should make changes to the code so that it never is true.
We don't allow mode changes in a transaction and we do not allow it if other users are actively using the database.

src/query/interpreter.hpp

andrejtonev · 2023-08-08T11:27:05Z

src/query/interpreter.hpp

 std::visit([](auto &memory_resource) { memory_resource.Release(); }, execution_memory);
 }
+
+ void CleanRuntimeData() {


Why is this needed?

andrejtonev · 2023-08-08T11:30:41Z

src/query/procedure/mg_procedure_impl.cpp

- if (auth_checker->Has(**impl_it, memgraph::query::AuthQuery::FineGrainedPrivilege::READ)) {
-  const auto &check_vertex =
-   it.source_vertex.getImpl() == (*impl_it)->From() ? (*impl_it)->To() : (*impl_it)->From();
+ auto edgeAcc = **impl_it;


Is this a copy?

not sure what was happening here, cc @Darych take a look please

as51340 · 2023-08-09T12:24:31Z

This merge has dropped the performance on certain benchmarks by some 10% (check single_vertex_read, single_edge_read, etc.) Double check if this is due to a fundamental memgraph change or something to do with the benchmark itself.

If it is 10% that is more-less ok I think. Buda expected that due to virtual calls.

antoniofilipovic · 2023-08-10T21:28:50Z

src/storage/v2/disk/storage.cpp

+ auto acc = vertices_.access();
+ auto vertex_it = acc.find(gid);
+ if (vertex_it != acc.end()) {
+ return VertexAccessor::Create(&*vertex_it, &transaction_, &storage_->indices_, &storage_->constraints_, config_,
+ view);
+ }
+ for (const auto &vec : index_storage_) {
+ acc = vec->access();
+ auto index_it = acc.find(gid);
+ if (index_it != acc.end()) {
+ return VertexAccessor::Create(&*index_it, &transaction_, &storage_->indices_, &storage_->constraints_, config_,
+ view);
+ }
+ }
+
+ rocksdb::ReadOptions read_opts;
+ auto strTs = utils::StringTimestamp(transaction_.start_timestamp);
+ rocksdb::Slice ts(strTs);
+ read_opts.timestamp = &ts;
+ auto *disk_storage = static_cast<DiskStorage *>(storage_);
+ auto it = std::unique_ptr<rocksdb::Iterator>(
+ disk_transaction_->GetIterator(read_opts, disk_storage->kvstore_->vertex_chandle));
+ for (it->SeekToFirst(); it->Valid(); it->Next()) {
+ const auto &key = it->key();
+ if (Gid::FromUint(std::stoull(utils::ExtractGidFromKey(key.ToString()))) == gid) {
+ return LoadVertexToMainMemoryCache(key, it->value());
+ }
+ }
+ return std::nullopt;
+}


I think in this function we check whether vertex exists in vertices_ first, then we check whethere there is vertex in index, then we get it from storage and load it to cache. But in loding to cache we again check if vertex exists in skip list and I think this repetition is costly.

antoniofilipovic · 2023-08-10T21:29:46Z

src/storage/v2/disk/storage.cpp

+ auto main_storage_accessor = vertices_.access();
+
+ const std::string key_str = key.ToString();
+ storage::Gid gid = Gid::FromUint(std::stoull(utils::ExtractGidFromKey(key_str)));
+ if (VertexExistsInCache(main_storage_accessor, gid)) {
+ return std::nullopt;
+ }


This function is called by FindVertex. FindVertex already does check whether it is contained in skip list.

I feel like this function should only do the loading, and check should be done before

antoniofilipovic · 2023-08-10T21:31:18Z

src/storage/v2/disk/storage.cpp

+ if (VertexExistsInCache(index_accessor, gid)) {
+ return std::nullopt;
+ }


I feel like this function shouldn't do this check. This check should be done before and then we just here load.

I know it is all intertwined but idea I think of this review is to separate it if possible

antoniofilipovic · 2023-08-10T21:32:15Z

src/storage/v2/disk/storage.cpp

+ const auto edge_parts = utils::Split(key.ToStringView(), "|");
+ const Gid edge_gid = Gid::FromUint(std::stoull(edge_parts[4]));


Can this be part of some util function?

antoniofilipovic

Passed through some stuff in disk storage

antoniofilipovic · 2023-08-10T21:32:55Z

src/storage/v2/disk/storage.cpp

+ auto edge_acc = edges_.access();
+ auto res = edge_acc.find(edge_gid);
+ if (res != edge_acc.end()) {
+ return std::nullopt;
+ }


Can we do that check before somewhere, so this function only does deserialization of edge from rocksdb

antoniofilipovic · 2023-08-10T21:33:36Z

src/storage/v2/disk/storage.cpp

+
+ const auto [from_gid, to_gid] = std::invoke(
+ [](const auto &edge_parts) {
+ if (edge_parts[2] == "0") { // out edge


Can this index and zero be some kind of constants?

antoniofilipovic · 2023-08-10T21:35:31Z

src/storage/v2/disk/storage.cpp

+ for (it->SeekToFirst(); it->Valid(); it->Next()) {
+ LoadVertexToMainMemoryCache(it->key(), it->value());
+ }


Can we return this as iterable, and then load vertices to main memory cache as something iterates over vertices?

antoniofilipovic · 2023-08-10T21:37:39Z

src/storage/v2/disk/storage.cpp

+ auto main_storage_accessor = vertices_.access();
+
+ const std::string key_str = key.ToString();
+ storage::Gid gid = Gid::FromUint(std::stoull(utils::ExtractGidFromKey(key_str)));
+ if (VertexExistsInCache(main_storage_accessor, gid)) {
+ return std::nullopt;
+ }


I feel like this function should only do the loading, and check should be done before

antoniofilipovic · 2023-08-10T21:38:44Z

src/storage/v2/disk/storage.cpp

+VerticesIterable DiskStorage::DiskAccessor::Vertices(LabelId label, View view) {
+ index_storage_.emplace_back(std::make_unique<utils::SkipList<storage::Vertex>>());
+ auto &indexed_vertices = index_storage_.back();
+ index_deltas_storage_.emplace_back(std::list<Delta>());


You can just pass empty()

But I think we resolved that in other PR

antoniofilipovic · 2023-08-10T21:39:46Z

src/storage/v2/disk/storage.cpp

+ if (VertexExistsInCache(index_accessor, gid)) {
+ return std::nullopt;
+ }


I know it is all intertwined but idea I think of this review is to separate it if possible

antoniofilipovic · 2023-08-10T21:42:22Z

src/storage/v2/disk/storage.cpp

+ std::string key_str = index_it->key().ToString();
+ std::string it_value_str = index_it->value().ToString();
+ Gid curr_gid = Gid::FromUint(std::stoull(utils::ExtractGidFromLabelPropertyIndexStorage(key_str)));
+ // TODO: andi this will be optimized bla bla


I think we shouldn't have this comment here :) Just TODO(andi) optimize

Add ON_DISK_TRANSACTIONAL storage

81a329c

gitbuda added this to the mg-v2.8.0 milestone Mar 29, 2023

gitbuda assigned Darych and as51340 Mar 29, 2023

gitbuda added the feature feature label Mar 29, 2023

gitbuda and others added 4 commits March 31, 2023 13:04

Merge branch 'master' into add-on-disk-transactional-storage

3f209e3

[E216 < T1245] Add subqueries (#794) (#851)

7fe17ba

Add subqueries Co-authored-by: Bruno Sačarić <bruno.sacaric@gmail.com>

Merge branch 'master' of github.com:memgraph/memgraph

cc765f0

Merge branch 'master' into add-on-disk-transactional-storage

da8aa68

gitbuda modified the milestones: mg-v2.8.0, mg-v2.9.0 Apr 17, 2023

Aidar Samerkhanov and others added 2 commits May 9, 2023 15:24

Merge branch 'master' into add-on-disk-transactional-storage

fa0c2bc

Merge branch 'master' into add-on-disk-transactional-storage

976c360

gitbuda added the Capability - on-disk label May 19, 2023

gitbuda changed the title ~~Add ON_DISK_TRANSACTIONAL storage~~ [master < E] Add ON_DISK_TRANSACTIONAL storage May 22, 2023

gitbuda changed the title ~~[master < E] Add ON_DISK_TRANSACTIONAL storage~~ [master < E850] Add ON_DISK_TRANSACTIONAL storage May 22, 2023

Merge branch 'master' into add-on-disk-transactional-storage

e07d165

gitbuda changed the title ~~[master < E850] Add ON_DISK_TRANSACTIONAL storage~~ [master < E850] Add ON_DISK_TRANSACTIONAL storage v1/experimental Jun 27, 2023

as51340 added 2 commits June 28, 2023 13:33

Add larger than memory local cache (#943)

d40ed88

Merge branch 'master' into add-on-disk-transactional-storage

523ecc5

gitbuda changed the title ~~[master < E850] Add ON_DISK_TRANSACTIONAL storage v1/experimental~~ Add experimental/v1 ON_DISK_TRANSACTIONAL storage Jun 28, 2023

Remaining clang-tidy issues

a1eb024

gitbuda marked this pull request as ready for review June 29, 2023 09:41

Merge branch 'master' into add-on-disk-transactional-storage

3f5c816

gitbuda merged commit 9d056e7 into master Jun 29, 2023

gitbuda deleted the add-on-disk-transactional-storage branch June 29, 2023 09:44

andrejtonev reviewed Aug 7, 2023

View reviewed changes

andrejtonev reviewed Aug 8, 2023

View reviewed changes

antoniofilipovic reviewed Aug 10, 2023

View reviewed changes

		const auto edge_parts = utils::Split(key.ToStringView(), "\|");
		const Gid edge_gid = Gid::FromUint(std::stoull(edge_parts[4]));

Add experimental/v1 ON_DISK_TRANSACTIONAL storage #850

Add experimental/v1 ON_DISK_TRANSACTIONAL storage #850

Uh oh!

Conversation

gitbuda commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

andrejtonev commented Aug 7, 2023

andrejtonev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

as51340 Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

andrejtonev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

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

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

Choose a reason for hiding this comment

Uh oh!

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 Aug 9, 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

antoniofilipovic 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

Choose a reason for hiding this comment

Labels

6 participants

gitbuda commented Mar 29, 2023 •

edited

Loading

as51340 Aug 9, 2023 •

edited

Loading