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

Disable updating fs cache during backup/restore. #52402

Merged
merged 6 commits into from Aug 3, 2023

Conversation

vitlibar
Copy link
Member

@vitlibar vitlibar commented Jul 21, 2023

Changelog category:

  • Improvement

Changelog entry:

Disable updating fs cache during backup/restore. Filesystem cache must not be updated during backup/restore, it seems it just slows down the process without any profit (because the BACKUP command can read a lot of data and it's no use to put all the data to the filesystem cache and immediately evict it).

@robot-ch-test-poll robot-ch-test-poll added the pr-improvement Pull request with some product improvements label Jul 21, 2023
@robot-ch-test-poll
Copy link
Contributor

robot-ch-test-poll commented Jul 21, 2023

This is an automated comment for commit 587877d with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🟡 pending

Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR🟡 pending
Mergeable CheckChecks if all other necessary checks are successful🟢 success
Push to DockerhubThe check for building and pushing the CI related docker images to docker hub🟢 success

@vitlibar vitlibar changed the title Disable using fs cache for backup/restore. Disable updating fs cache during backup/restore. Jul 21, 2023
@kssenii kssenii self-assigned this Jul 21, 2023
Comment on lines 189 to 190
read_settings.enable_filesystem_cache = false;
read_settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache = backup_settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache;
Copy link
Member

Choose a reason for hiding this comment

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

setting read_from_filesystem_cache_if_exists_otherwise_bypass_cache will not work if enable_filesystem_cache is disabled, see

return settings.remote_fs_cache && settings.enable_filesystem_cache
&& (!CurrentThread::getQueryId().empty() || settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache
|| !settings.avoid_readthrough_cache_outside_query_context);

may be better something like this?

read_settings.enable_filesystem_cache = backup_settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache;
read_settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache = backup_settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache;
read_settings.avoid_readthrough_cache_outside_query_context = false; /// not sure if needed

Copy link
Member Author

@vitlibar vitlibar Jul 24, 2023

Choose a reason for hiding this comment

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

Thank you!

avoid_readthrough_cache_outside_query_context doesn't seem necessary because though BACKUP/RESTORE can operate in background they always have proper query id. Anyway the tests pass.

Copy link
Member

@kssenii kssenii left a comment

Choose a reason for hiding this comment

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

also may be add a test?

@vitlibar vitlibar force-pushed the disable-fs-cache-for-backups branch 2 times, most recently from a7c0417 to 11cdb8c Compare July 24, 2023 11:35
@vitlibar
Copy link
Member Author

also may be add a test?

I added test.

@vitlibar vitlibar force-pushed the disable-fs-cache-for-backups branch from a4812fa to 574eac8 Compare July 25, 2023 00:18
@vitlibar vitlibar force-pushed the disable-fs-cache-for-backups branch from 6cd1a9f to 6ac61b1 Compare August 3, 2023 11:53
@vitlibar
Copy link
Member Author

vitlibar commented Aug 3, 2023

@vitlibar vitlibar merged commit 75b553b into ClickHouse:master Aug 3, 2023
10 of 13 checks passed
@vitlibar vitlibar deleted the disable-fs-cache-for-backups branch August 3, 2023 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants