DatabaseAtomic by tavplubix · Pull Request #7512 · ClickHouse/ClickHouse

tavplubix · 2019-10-28T20:57:20Z

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

For changelog. Remove if this is non-significant change.

Category (leave one):

New Feature

Short description (up to few sentences):
Added experimental database engine Atomic. It supports non-blocking DROP and RENAME TABLE queries and atomic EXCHANGE TABLES t1 AND t2 query
#6787

KochetovNicolai · 2020-04-23T14:10:22Z

src/Interpreters/loadMetadata.cpp

        databases.emplace(unescapeForFileName(it.name()), it.path().toString());
    }

+    if (!default_database_name.empty() && !databases.count(default_database_name))


Please, add comment.

KochetovNicolai · 2020-04-23T15:13:47Z

src/Common/rename.cpp

+
+static void renameat2(const std::string & old_path, const std::string & new_path, int flags)
+{
+    if (old_path.empty() || new_path.empty())


Please, use more detailed exception message.
Like Cannot rename old_path to new_path because one of them is empty or Cannot rename old_path to new_path because one of them is absolute.

KochetovNicolai · 2020-04-23T15:15:21Z

src/Common/rename.cpp

@@ -0,0 +1,122 @@
+#include <Common/rename.h>


I think file name is too general.
At least better to use renameDirectory.h or something.

KochetovNicolai · 2020-04-23T15:17:55Z

src/Common/rename.cpp

+        return;
+
+    if (errno == EEXIST)
+        throwFromErrnoWithPath("Cannot RENAME_NOREPLACE " + old_path + " to " + new_path, new_path, ErrorCodes::ATOMIC_RENAME_FAIL);


Cannot rename old_path to new_path because path is already exists.

Please, add the comment to throwFromErrnoWithPath declaration in Exception.h

KochetovNicolai · 2020-04-23T15:20:27Z

src/Common/rename.cpp

+    if (errno == EEXIST)
+        throwFromErrnoWithPath("Cannot RENAME_NOREPLACE " + old_path + " to " + new_path, new_path, ErrorCodes::ATOMIC_RENAME_FAIL);
+    if (errno == ENOENT)
+        throwFromErrno("Paths cannot be exchanged because " + old_path + " or " + new_path + " does not exist", ErrorCodes::ATOMIC_RENAME_FAIL);


Good comment.
But why we don't use throwFromErrnoWithPath now?

In case of RENAME_EXCHANGE we cannot choose one of them. However, throwFromErrnoWithPath is useful only to add information about available space and inodes to exception message

KochetovNicolai · 2020-04-23T15:23:35Z

src/Common/rename.cpp

+/// Do not use [[noreturn]] to avoid warnings like "code will never be executed" in other places
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wmissing-noreturn"
+static void renameExchangeFallback(const std::string &, const std::string &)


Do we need this function?
maybe throw exception right into renameExchange

We need it to suppress warnings like declaration missing '[[noreturn]]' and code will never be executed in MacOS build only for this exception and this function.

KochetovNicolai · 2020-04-23T15:46:28Z

src/Databases/DatabaseAtomic.h

+
+    void loadStoredObjects(Context & context, bool has_force_restore_data_flag) override;
+
+    void assertCanBeDetached(bool cleenup);


Comment is needed.

KochetovNicolai · 2020-04-23T16:05:03Z

src/Databases/DatabaseAtomic.cpp

+
+
+DatabaseAtomic::DatabaseAtomic(String name_, String metadata_path_, Context & context_)
+    : DatabaseOrdinary(name_, metadata_path_, "store/", "DatabaseAtomic (" + name_ + ")", context_)


std::move for name_ and metadata_path_

name_ cannot be moved because it's also used in other argument

azat · 2020-05-17T00:36:52Z

Looks like this can lead to SIGSEGV on parallel ALTER? See https://clickhouse-test-reports.s3.yandex.net/10927/c059ee008519cff43b42022fc6f6a9d8ee86c56c/functional_stateless_tests_(release,_databaseatomic).html#fail1

tavplubix · 2020-05-17T01:44:20Z

It's SIGABRT caused by std::terminate() call when ALTER tries to rewrite metadata after database was dropped.
#10968

winter7 · 2020-06-02T09:38:46Z

Can we "alter" a database to Atomic engine?

tavplubix · 2020-06-09T06:24:23Z

Can we "alter" a database to Atomic engine?

As for now it can be done only manually by modifying metadata files and moving data of tables. Also it's possible to RENAME TABLE db_ordinary.table_name TO db_atomic.table_name. But notice that Atomic engine is experimetal and backward compatibility may be broken in future versions. Ability to alter a database from Ordinary to Atomic engine will be probably added later.

winter7 · 2020-06-09T07:21:59Z

Thanks! I think I will wait for the official ALTER release and keep an eye on this engine.

tavplubix added 4 commits October 23, 2019 16:46

add UUID to ASTCreateQuery

3eaa96e

refactor InterpreterCreateQuery::createTable(...)

9c461ff

make data path relative

80c9b2e

use arbitrary relative path in *MergeTree

ec7de6d

stavrolia added the pr-feature Pull request with new product feature label Oct 28, 2019

tavplubix added 25 commits October 29, 2019 17:11

Merge branch 'master' into database_atomic

5059c78

fixes

a3b4762

refactor StorageFile construction

9bbeb63

refactor IDatabase

d0af0c8

refactor databases: add DatabaseWithDictionaries

b686738

refactor databases: inherit from DatabaseOnDisk

14e8715

Merge branch 'master' into database_atomic

d4a065c

fixes

075edf0

use UUIDs

ab4d452

refactor IStorage::rename(...)

cff9b02

Merge branch 'master' into database_atomic

a6fe357

Merge branch 'master' into database_atomic

e44338f

fixes

c0d4d19

attempt to run tests

8cba35b

try fix backward compatibility

80e082d

fix

8bdc855

rename without table lock

268c8e7

Merge branch 'master' into database_atomic

4a5c4bc

refactor storage name

b047e7b

better constructors of storages

8e319ac

refactor Context 1

0bc2fab

refactor Context 2

03b1a57

use StorageID in views

0b70bff

remove separate getTableName() and getDatabaseName()

3a4ee4a

Merge branch 'master' into database_atomic

4d23c5e

fix

e02b579

KochetovNicolai reviewed Apr 23, 2020

View reviewed changes

Merge branch 'master' into database_atomic

04d6b59

KochetovNicolai reviewed Apr 23, 2020

View reviewed changes

tavplubix added 3 commits April 23, 2020 19:51

remove Context from some IDatabase methods

f413061

fixes

15e7ce0

fix build

8be4d51

tavplubix removed the pr-improvement Pull request with some product improvements label Apr 23, 2020

tavplubix added 2 commits April 24, 2020 01:08

Update DatabaseCatalog.cpp

7d6ab32

update yamake

04d4130

tavplubix merged commit 7928f2e into master Apr 24, 2020

tavplubix deleted the database_atomic branch April 24, 2020 19:28

This was referenced Apr 26, 2020

DatabaseCatalog is not initialized #10208

Closed

Caught exception while loading metadata，Unable to launch clickhouse #7005

Closed

azat mentioned this pull request May 17, 2020

Database atomic fixes #10980

Merged

alexey-milovidov mentioned this pull request Jun 6, 2020

Miscellaneous changes for SystemLogs #11293

Closed

tavplubix restored the database_atomic branch September 24, 2020 23:07

tavplubix deleted the database_atomic branch September 26, 2020 23:43

azat mentioned this pull request Oct 5, 2025

Fix uncaught exception due noexcept tryGetCreateTableQuery() #88105

Merged


		void loadStoredObjects(Context & context, bool has_force_restore_data_flag) override;

		void assertCanBeDetached(bool cleenup);



		DatabaseAtomic::DatabaseAtomic(String name_, String metadata_path_, Context & context_)
		: DatabaseOrdinary(name_, metadata_path_, "store/", "DatabaseAtomic (" + name_ + ")", context_)

Conversation

tavplubix commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tavplubix Apr 23, 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!

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!

azat commented May 17, 2020

Uh oh!

tavplubix commented May 17, 2020

Uh oh!

winter7 commented Jun 2, 2020

Uh oh!

tavplubix commented Jun 9, 2020

Uh oh!

winter7 commented Jun 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tavplubix commented Oct 28, 2019 •

edited

Loading

tavplubix Apr 23, 2020 •

edited

Loading