-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
fix hang up with command 'drop table system.query_log sync' #33293
fix hang up with command 'drop table system.query_log sync' #33293
Conversation
I don't completely know what logic here should be, however, don't think it is a correct fix. The idea that we don't delete storage until there is any reference to storage object. And executing select definitely should have one. |
Normally, the storage will not be deleted until there is any reference to storage object. |
@Mergifyio update |
✅ Branch has been successfully updated |
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.
The fix LGTM, but it would be great to add an integration test
75c1250
to
47b6e2d
Compare
Thanks. |
47b6e2d
to
c5aa0bd
Compare
…- add test case" This reverts commit c5aa0bd.
I'm not sure how or why, but this PR fixed a crash when shutting down CH after a manual ALTER in system.query_log. Without this fix something like this would cause fatal errors around 50% of the times (beware it's shutting down the server):
Backtrace in debug mode to get a clearer stack:
For sure it affected official builds from 21.9 to 21.12 included. Similar tickets in #30388 and |
Changelog category (leave one):
Detailed description / Documentation draft:
Problem phenomenon:
When I use 'drop table system.query_log sync', the client will hangup.
Problem root cause:
When called to dropTableDataTask() in process of drop table, the table.unique() return false. This is because 'table' in Class SystemLog does not free the smart pointer.