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.

Sign up for GitHub

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

Jump to bottom

Do not reload *all* dictionaries on CREATE DATABASE #7916

Merged
merged 1 commit into from
Dec 2, 2019
Merged

Do not reload *all* dictionaries on CREATE DATABASE #7916

merged 1 commit into from
Dec 2, 2019

Conversation

azat
Copy link
Collaborator

@azat azat commented Nov 25, 2019

Changelog category (leave one):

  • Bug Fix

Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):
Do not reload all dictionaries on CREATE DATABASE (This ignores any lifetime, while dictionaries can be quite big).

Detailed description (optional):

Fixes: c7cd911 ("Merge pull request #7360")
Refs: #7360 (comment)

Cc: @alesapin

@azat azat force-pushed the dict-avoid-RELOAD-on-CREATE-DATABASE branch 3 times, most recently from d9755f6 to 6982685 Compare November 26, 2019 07:16
@azat
Do not reload *all* dictionaries on CREATE DATABASE
4530ade 
This ignores any lifetime, while dictionaries can be quite big.

Fixes: c7cd911 ("Merge pull request ClickHouse#7360")
Refs: ClickHouse#7360 (comment)
@azat azat force-pushed the dict-avoid-RELOAD-on-CREATE-DATABASE branch from 6982685 to 4530ade Compare November 26, 2019 07:33
@stavrolia stavrolia added can be tested pr-bugfix Pull request with bugfix, not backported by default labels Nov 26, 2019
@vitlibar vitlibar self-assigned this Nov 27, 2019
@alesapin
Copy link
Member

lgtm

@alesapin alesapin merged commit af85eb8 into ClickHouse:master Dec 2, 2019
nikitamikhaylov pushed a commit that referenced this pull request Dec 2, 2019
@alesapin @nikitamikhaylov
Merge pull request #7916 from azat/dict-avoid-RELOAD-on-CREATE-DATABASE
196cfbd 
Do not reload *all* dictionaries on CREATE DATABASE

(cherry picked from commit af85eb8)
@nikitamikhaylov
Copy link
Member

Conflicts with 19.16.

@azat azat deleted the dict-avoid-RELOAD-on-CREATE-DATABASE branch December 5, 2019 10:02
@alexey-milovidov
Copy link
Member

Tests are bad:

https://clickhouse-test-reports.s3.yandex.net/8086/9399b39070296951f03224696edd3819d354ba98/functional_stateless_tests_(unbundled).html

https://clickhouse-test-reports.s3.yandex.net/8086/9399b39070296951f03224696edd3819d354ba98/functional_stateless_tests_(ubsan).html

@azat
Copy link
Collaborator Author

azat commented Dec 9, 2019

Tests are bad:

Will take a look

@azat
Copy link
Collaborator Author

azat commented Dec 9, 2019

The problem is that SYSTEM RELOAD DICTIONARY is async

2019.12.09 05:07:21.862801 [ 36 ] {61df55ee-4a17-4bf3-8cfb-9478490393c3} <Debug> executeQuery: (from [::1]:37422) SYSTEM RELOAD DICTIONARY `foo 1234.dict`
2019.12.09 05:07:21.863289 [ 36 ] {61df55ee-4a17-4bf3-8cfb-9478490393c3} <Debug> MemoryTracker: Peak memory usage (for query): 0.00 B.
2019.12.09 05:07:21.863334 [ 36 ] {61df55ee-4a17-4bf3-8cfb-9478490393c3} <Debug> MemoryTracker: Peak memory usage (total): 0.00 B.
2019.12.09 05:07:21.863358 [ 36 ] {61df55ee-4a17-4bf3-8cfb-9478490393c3} <Information> TCPHandler: Processed in 0.001 sec.
2019.12.09 05:07:21.863681 [ 36 ] {cd55d9f9-d176-4b19-bc97-d25575da2d15} <Debug> executeQuery: (from [::1]:37422) SELECT query_count FROM system.dictionaries WHERE (database = 'foo 1234') AN>
2019.12.09 05:07:21.863678 [ 54 ] {} <Debug> executeQuery: (internal) SELECT `key`, `val` FROM `foo 1234`.`dict_data`;

To fix this properly I guess something like SYSTEM SYNC RELOAD DICTIONARY/DICTIONARIES should be added, thoughts?

As a hot-fix, sleep(1) or polling system.dictionaries can be added

@alexey-milovidov
Copy link
Member

I think, it's better to make SYSTEM RELOAD DICTIONARY and DICTIONARIES always synchronous.

As a hot-fix, sleep(1)

It's ok for hotfix.

azat added a commit to azat/ClickHouse that referenced this pull request Dec 10, 2019
@azat
Fix async nature of reload in 01036_no_superfluous_dict_reload_on_cre…
65572a2 
…ate_database*

Due to the async nature of SYSTEM RELOAD DICTIONARY/DICTIONARIES the
test can fail, if the reload will happen after the dictGet*():

    .862801 [ 36 ] {61df55ee-4a17-4bf3-8cfb-9478490393c3} <Debug> executeQuery: (from [::1]:37422) SYSTEM RELOAD DICTIONARY `foo 1234.dict`
    .863681 [ 36 ] {cd55d9f9-d176-4b19-bc97-d25575da2d15} <Debug> executeQuery: (from [::1]:37422) SELECT query_count FROM system.dictionaries WHERE (database = 'foo 1234') AND ...
    .863678 [ 54 ] {} <Debug> executeQuery: (internal) SELECT `key`, `val` FROM `foo 1234`.`dict_data`;

This is just a hotfix, long-term solution will be to make SYSTEM RELOAD
DICTIONARY syncronous (by adding another command or making already
existing).

Refs: ClickHouse#7916
@azat azat mentioned this pull request Dec 10, 2019
Merged
@azat azat mentioned this pull request Apr 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov left review comments

@alesapin alesapin alesapin approved these changes

Assignees

@vitlibar vitlibar

Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@azat @alesapin @nikitamikhaylov @alexey-milovidov @stavrolia @vitlibar