Repository navigation
Allow to prewarm marks of secondary indexes - #97772
Conversation
azat
left a comment
There was a problem hiding this comment.
Looks good.
I guess it does not make sense to introduce separate setting to control skip indexes prewarm, since they will be loaded during reading anyway. What do you think?
|
I'd like to keep the existing setting to control both prewarms. Moreover, I think that a separate cache for skip index marks is a mistake. |
|
Can we backport it to 26.2? |
|
I don't mind backporting to 26.2. |
| if ((*settings)[MergeTreeSetting::prewarm_mark_cache]) | ||
| { | ||
| result.mark_cache = getContext()->getMarkCache(); | ||
| result.index_mark_cache = getContext()->getIndexMarkCache(); |
There was a problem hiding this comment.
I was thinking that we may have a separate prewarm_index_mark_cache setting. However, looks like it's covered by index_mark_cache_prewarm_ratio = 0.
Maybe here set index_mark_cache=nullptr if index_mark_cache_prewarm_ratio = 0, but it's a minor thing.
There was a problem hiding this comment.
I'd like to keep one setting. As a user, I would expect that index marks are prewarmed with prewarm_index_mark_cache. As I said, I think that separation of the mark cache and the index mark cache was a mistake.
| ::: | ||
| )", 0) \ | ||
| DECLARE(Double, index_mark_cache_size_ratio, DEFAULT_INDEX_MARK_CACHE_SIZE_RATIO, R"(The size of the protected queue (in case of SLRU policy) in the secondary index mark cache relative to the cache's total size.)", 0) \ | ||
| DECLARE(Double, index_mark_cache_prewarm_ratio, 0.95, R"(The ratio of total size of index mark cache to fill during prewarm.)", 0) \ |
There was a problem hiding this comment.
We don't have compatibility for ServerSettings. Does it look fine to use 0.95 by default?
Probably fine with me; we can just fill the index mark cache's memory faster...
| continue; | ||
|
|
||
| auto marks_file = index_granularity_info.getMarksFilePath(*stream_name); | ||
| auto key = MarkCache::hash(getDataPartStorage().getDiskName() + ":" + (fs::path(getRelativePathOfActivePart()) / marks_file).string()); |
There was a problem hiding this comment.
I wonder why we leaked the internals of mark loader from here
But at least this PR does not make it worse; it was copied many times in MergeTreeDataPart*::removeMarksFromCache already.
There was a problem hiding this comment.
It's quite dirty, but there probably isn't a better way to do it. Maybe only move this to a static method of MergeTreeMarksLoader.
Backport #97772 to 26.2: Allow to prewarm marks of secondary indexes
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Marks of secondary indexes are now prewarmed when the
prewarm_mark_cache settingis enabled (loaded into the index mark cache during data part fetches and table startup).Version info
26.3.1.10126.2.1.1135