Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support memory only mode for StorageJoin and StorageSet. #14776

Merged

Conversation

Vxider
Copy link
Contributor

@Vxider Vxider commented Sep 13, 2020

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):

Support for disabling persistency for StorageJoin and StorageSet, this feature is controlled by setting disable_set_and_join_persistency. And this PR solved issue #6318.

Detailed description / Documentation draft:

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Sep 13, 2020
@@ -229,8 +242,24 @@ void registerStorageSet(StorageFactory & factory)
"Engine " + args.engine_name + " doesn't support any arguments (" + toString(args.engine_args.size()) + " given)",
ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH);

return StorageSet::create(args.relative_data_path, args.table_id, args.columns, args.constraints, args.context);
});
const auto & settings = args.context.getSettingsRef();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why support both query-level and table-level settings?
Usage of query-level settings is inconvenient for CREATE TABLE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd stick with table-level settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a new commit that only use table-level settings. And also add functional tests.

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add a test.
A functional test with DEATCH/ATTACH TABLE should suffice.

@alexey-milovidov alexey-milovidov self-assigned this Sep 14, 2020
@Vxider Vxider force-pushed the disable_set_and_join_persistency branch from 2114f4a to e136eba Compare September 15, 2020 11:00
@alexey-milovidov
Copy link
Member

There is something related to DatabaseAtomic...

Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have two identical settings files for Set and Join.
This is reasonable if we expect to have more settings that will be distinct for Set and Join. Ok...


CREATE TABLE join (k UInt64, s String) ENGINE = Join(ANY, LEFT, k) SETTINGS disable_persistency=1;

DETACH TABLE join;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did I mean... we can insert some data,
validate that it will be present after DETACH/ATTACH if disable_persistency = 0
and validate that table will be empty after DETACH/ATTACH otherwise.

PS. Maybe better to name a setting persistent with default value 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a commit to rename disable_persistency to persistent with default value 1. And update the tests.

@alexey-milovidov
Copy link
Member

@tavplubix about DETACH/ATTACH sequence in DatabaseAtomic - is there any caveats, what should we do?

@tavplubix
Copy link
Member

@tavplubix about DETACH/ATTACH sequence in DatabaseAtomic - is there any caveats, what should we do?

Test failure can be fixed by using short attach syntax (ATTACH TABLE set; instead of ATTACH TABLE set (x String) ENGINE = Set() SETTINGS disable_persistency=1;). Another way to fix it is to specify table UUID explicitly (e.g. CREATE TABLE set UUID '00001493-1000-4000-8000-000000000001' (x String) ENGINE = Set() SETTINGS disable_persistency=1;) and use the same UUID in ATTACH query.

As for DETACH/ATTACH sequence, it should work fine if no queries are using the table after DETACH. Otherwise, Cannot attach table exception will be thrown to avoid race condition.

@tavplubix
Copy link
Member

Probably we should allow attaching another instance of storage if it doesn't store data on disk, but I'm not sure.

@alexey-milovidov
Copy link
Member

test_rename_column

Already fixed.

@alexey-milovidov alexey-milovidov merged commit b6bccfc into ClickHouse:master Sep 30, 2020
@Vxider Vxider deleted the disable_set_and_join_persistency branch October 29, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants