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
Allow KILL QUERY for backups #58804
Allow KILL QUERY for backups #58804
Conversation
57ec4b9
to
5eafd54
Compare
This is an automated comment for commit 1a1b089 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
… BACKUP/RESTORE query started by user and its asynchronous execution in a separate thread.
ddc0c14
to
d3b3d30
Compare
to set whether shutdown should wait for running backups to finish or just cancel them.
d3b3d30
to
4c6d3e7
Compare
@novikd This PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM
if (!storage) | ||
continue; | ||
|
||
checkQueryNotCancelled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be done before checking if storage
is a null pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm calling checkQueryNotCancelled()
before doing anything time-consuming. Checking a pointer for null is not time-consuming.
src/Backups/BackupEntriesCollector.h
Outdated
@@ -97,11 +100,15 @@ class BackupEntriesCollector : private boost::noncopyable | |||
|
|||
Strings setStage(const String & new_stage, const String & message = ""); | |||
|
|||
/// Throws an exception if the BACKUP query was cancelled. | |||
void checkQueryNotCancelled() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename to checkQueryIsCancelled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to checkIsQueryCancelled()
src/Backups/BackupsWorker.cpp
Outdated
bool called_async) | ||
{ | ||
std::optional<CurrentThread::QueryScope> query_scope; | ||
SCOPE_EXIT_SAFE( | ||
if (called_async) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also check if thread_group != nullptr
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/Backups/BackupsWorker.cpp
Outdated
bool called_async) | ||
{ | ||
std::optional<CurrentThread::QueryScope> query_scope; | ||
SCOPE_EXIT_SAFE( | ||
if (called_async) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
SCOPE_EXIT({ | ||
DENY_ALLOCATIONS_IN_SCOPE; | ||
|
||
auto lock = unsafeLock(); | ||
elem->is_cancelling = false; | ||
cancelled_cv.notify_all(); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move it to the function beginning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks more logically consistent now - because first we set is_cancelling
to true, then we call cancelQuery(kill)
and set is_cancelling
to false. Anyway the difference is not essential because safeLock()
should never throw.
Test failures: |
Changelog category:
Changelog entry:
Allow
KILL QUERY
to cancel backups / restores. This PR also makes running backups and restores visible insystem.processes
. Also there is a new setting in the server configuration now -shutdown_wait_backups_and_restores
(default=true) which makes the server either wait on shutdown for all running backups and restores to finish or just cancel them.