Add mutation support for StorageMemory by ucasfl · Pull Request #15127 · ClickHouse/ClickHouse

ucasfl · 2020-09-22T09:33:39Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

fix #9117

Detailed description / Documentation draft:

...

By adding documentation, you'll allow users to try your new feature immediately, not when someone else will have time to document it later. Documentation is necessary for all features that affect user experience in any way. You can add brief documentation draft above, or add documentation right into your patch as Markdown files in docs folder.

If you are doing this for the first time, it's recommended to read the lightweight Contributing to ClickHouse Documentation guide first.

Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/

fix fix

…tation-for-storagememory

Akazz · 2020-09-29T15:23:13Z

Hey there, @ucasfl!
Currently StorageMemory uses the following mechanisms for resolving possible conflicts between concurrent operations:

SELECTs vs DROP/TRUNCATEs are resolved via table-level locks (lockForShare() vs lockExclusively())
SELECTs vs INSERTs and INSERTs vs DROP/TRUNCATEs are resolved via internal mutex

Implementing mutate() method would be very neat and we really appreciate your effort, but what you currently suggest in this PR is just not enough. And that is due to the fact that the current code cannot be easily extended to support efficient resolving possible conflicts of type MUTATEs vs <anything else>. One would either need to copy the whole list of blocks (in other words to implement some sort of versioned storage), or to put in some blocking synchronization with exclusiveLock(), and the latter is much less desirable.

alexey-milovidov · 2020-09-30T01:54:05Z

We have discussed it and the variant with copying the list of blocks to readers looks Ok.

ucasfl · 2020-10-01T05:07:54Z

If implement multiversion storage, do we still need table-level locks for storagememory? It doesn't seem to be needed anymore?@alexey-milovidov

alexey-milovidov · 2020-10-03T14:07:58Z

If implement multiversion storage, do we still need table-level locks for storagememory? It doesn't seem to be needed anymore?

Yes, there will be no table-level locks inside StorageMemory.
We only need a mutex, under which we will copy the list of blocks.

ucasfl · 2020-10-04T16:45:52Z

Hi, I have added MultiVersion storage in StorageMemory, you can take a look at it if you have time. @Akazz @alexey-milovidov

…tation-for-storagememory

Akazz

There seems to exist a race condition when concurrent INSERTs each try to add new blocks to their own copy of data.

Akazz · 2020-10-06T15:32:44Z

src/Storages/StorageMemory.cpp

+        auto new_data = std::make_unique<BlocksList>(*(storage.data.get()));
+        new_data->push_back(block);
+        storage.data.set(std::move(new_data));


There is going to be a problem due to a race condition here. Because we do not employ any CAS technique here, data can be lost when concurrent INSERTs each add a block to its own copy of data.

So, in order to solve this problem, we still need a table-level lock? @Akazz

…tation-for-storagememory

ucasfl · 2020-10-08T11:31:53Z

If implement multiversion storage, do we still need table-level locks for storagememory? It doesn't seem to be needed anymore?

Yes, there will be no table-level locks inside StorageMemory.
We only need a mutex, under which we will copy the list of blocks.

It seems that we still need a table-level lock to resolve concurrent mutation/insert. @alexey-milovidov

fix

…tation-for-storagememory

alexey-milovidov · 2020-11-23T01:55:35Z

src/Storages/StorageMemory.cpp

 void StorageMemory::truncate(
    const ASTPtr &, const StorageMetadataPtr &, const Context &, TableExclusiveLockHolder &)
 {
    std::lock_guard lock(mutex);


Looks like mutex is not needed anymore?

alexey-milovidov · 2020-11-23T01:57:21Z

src/Storages/StorageMemory.cpp

@@ -102,7 +106,9 @@ class MemoryBlockOutputStream : public IBlockOutputStream
        metadata_snapshot->check(block, true);
        {
            std::lock_guard lock(storage.mutex);


Here the mutex also looks unneeded.

I think we need it to resolve concurrent insert, mutation, drop...

I'm not sure... Concurrent INSERT and mutation is Ok: mutation will mutate previously inserted data and concurrently inserted data will be left unmutated.

For DROP there is table-level lock: IStorage::lockExclusively.

Let's write a test for race-condition (you can find similar .sh tests).

We already have race-condition test 01454_storagememory_data_race_challenge.sh.

alexey-milovidov · 2020-11-23T01:58:45Z

src/Storages/StorageMemory.cpp

@@ -201,17 +208,83 @@ BlockOutputStreamPtr StorageMemory::write(const ASTPtr & /*query*/, const Storag
 void StorageMemory::drop()
 {
    std::lock_guard lock(mutex);


Here mutex also looks unneeded.

alexey-milovidov · 2020-11-23T02:29:25Z

src/Storages/StorageMemory.cpp

+        auto new_data = std::make_unique<BlocksList>(*(data.get()));
+        auto data_it = new_data->begin();
+        auto out_it = out.begin();
+        while (data_it != new_data->end() && out_it != out.end())


Check that data has the same number of blocks?

Clarify in comments that deletion of whole blocks cannot happen here.

alexey-milovidov · 2020-11-23T02:30:58Z

src/Storages/StorageMemory.cpp

+            rows += buffer.rows();
+            bytes += buffer.bytes();
+        }
+        data.set(std::make_unique<BlocksList>(out));


Let's move calculation of rows/bytes and assignment out of the branch to avoid code duplication.

alexey-milovidov · 2020-11-23T02:33:15Z

src/Storages/StorageMemory.cpp

+void StorageMemory::mutate(const MutationCommands & commands, const Context & context)
+{
+    std::lock_guard lock(mutex);
+    auto metadata_snapshot = getInMemoryMetadataPtr();


Please add a test checking that ALTER ADD/REMOVE column is not supported right now (that is Ok).

alexey-milovidov

LGTM, only a few changes remain.

…tation-for-storagememory

alexey-milovidov · 2020-11-26T19:22:56Z

Alright.

add mutation support for StorageMemory

5d7a77c

robot-clickhouse added the pr-improvement Pull request with some product improvements label Sep 22, 2020

ucasfl force-pushed the add-mutation-for-storagememory branch from 322d4dc to ec683fe Compare September 22, 2020 10:42

add test

c0b5f84

fix fix

ucasfl force-pushed the add-mutation-for-storagememory branch from ec683fe to c0b5f84 Compare September 22, 2020 12:10

Akazz self-assigned this Sep 22, 2020

Merge branch 'master' of github.com:ClickHouse/ClickHouse into add-mu…

504d577

…tation-for-storagememory

ucasfl added 2 commits October 5, 2020 00:28

multiversion storage for StorageMemory

6289f44

add test

5400a41

Merge branch 'master' of github.com:ClickHouse/ClickHouse into add-mu…

55247bf

…tation-for-storagememory

Akazz suggested changes Oct 6, 2020

View reviewed changes

Akazz added the st-waiting-for-fix label Oct 7, 2020

ucasfl added 5 commits October 8, 2020 15:19

fix

3690294

Merge branch 'master' of github.com:ClickHouse/ClickHouse into add-mu…

23fa408

…tation-for-storagememory

fix

3bcb9b8

fix

ff1a314

format

ae54494

fix

677787f

fix

ucasfl force-pushed the add-mutation-for-storagememory branch from f7dab0b to 677787f Compare October 8, 2020 13:37

Merge branch 'master' of github.com:ClickHouse/ClickHouse into add-mu…

8d1780c

…tation-for-storagememory

ucasfl mentioned this pull request Nov 20, 2020

Support Mutations in storage Memory. #9117

Closed

abyss7 self-requested a review November 20, 2020 11:47

alexey-milovidov mentioned this pull request Nov 23, 2020

Improve efficiency of reading from Memory tables. #17277

Closed

alexey-milovidov unassigned Akazz Nov 23, 2020

alexey-milovidov self-assigned this Nov 23, 2020

alexey-milovidov removed the st-waiting-for-fix label Nov 23, 2020

alexey-milovidov reviewed Nov 23, 2020

View reviewed changes

ucasfl and others added 3 commits November 23, 2020 05:30

fix

18491d8

Merge branch 'master' of github.com:ClickHouse/ClickHouse into add-mu…

005f345

…tation-for-storagememory

Update StorageMemory.cpp

084cac5

alexey-milovidov approved these changes Nov 23, 2020

View reviewed changes

Update StorageMemory.cpp

ee80ee7

alexey-milovidov merged commit 1cd09fa into ClickHouse:master Nov 26, 2020

plugine mentioned this pull request May 17, 2021

Fix wrong typo at StorageMemory #24193

Merged

Conversation

ucasfl commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akazz commented Sep 29, 2020

Uh oh!

alexey-milovidov commented Sep 30, 2020

Uh oh!

ucasfl commented Oct 1, 2020

Uh oh!

alexey-milovidov commented Oct 3, 2020

Uh oh!

ucasfl commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Akazz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ucasfl Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ucasfl commented Oct 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov commented Nov 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ucasfl commented Sep 22, 2020 •

edited

Loading

ucasfl commented Oct 4, 2020 •

edited

Loading

ucasfl Oct 7, 2020 •

edited

Loading