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
Add system.dropped_tables_parts table - second attempt #58038
Add system.dropped_tables_parts table - second attempt #58038
Conversation
This is an automated comment for commit b05211c 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
|
@antonio2368 could you please specify which stateless tests were failing after this PR? |
@yakov-olkhovskiy all test failed here would consistently fail with Databse ordinary Seems we run with that database only on master so you couldn't have noticed that in PR. |
We also check that new (and midified) tests work with Ordinary database in the flaky check, but the modified
and the previous PR did not have any tests at all |
Btw, what's the usecase for the new table? |
StoragesDroppedInfoStream::StoragesDroppedInfoStream(const SelectQueryInfo & query_info, ContextPtr context) | ||
: StoragesInfoStreamBase(context) | ||
{ | ||
needsLock = false; |
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 ugly, however, acquiring the lock for share (especially in Atomic database, especially if the table is dropped) is almost no-op (no chance it will wait for something)
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.
this class is common for both - dropped and alive tables - we need the way to distinct between just-dropped table for system.parts and table for system.dropped_tables_parts
but yes, it's ugly - will try to improve
@@ -1,3 +1,4 @@ | |||
#include "Common/SipHash.h" |
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.
#include "Common/SipHash.h" | |
#include <Common/SipHash.h> |
UUID storage_uuid = storage->getStorageID().uuid; | ||
if (database->getEngineName() == "Ordinary") | ||
{ | ||
SipHash hash; | ||
hash.update(database_name); | ||
hash.update(table_name); | ||
storage_uuid = hash.get128(); |
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 should not display any UUIDs for tables from Ordinary databases, otherwise, it may be confusing. Also, assigning a random hash to UUID is wrong, because this UUID is likely to be invalid, see https://en.wikipedia.org/wiki/Universally_unique_identifier#Version_4_(random)
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.
we don't display this UUID - we need it only as a key for internal map - previously we used database_name+table_name pair here, but because we can delete table with the same name multiple times, it can't be used to uniquely identify such tables in dropped_tables, so I replaced this pair with table's UUID
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 confused here, wouldn't we also get the same hash for the same database - table name pair and still have the same problem for Ordinary databases as without UUID?
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.
@antonio2368 yes, we would, but per my knowledge Ordinary do not use postponed deletion
DROP TABLE 25400_dropped_tables; | ||
|
||
SELECT table, engine FROM system.dropped_tables WHERE database = currentDatabase() LIMIT 1; | ||
DESCRIBE TABLE system.dropped_tables; | ||
|
||
SELECT database, table, name FROM system.dropped_tables_parts WHERE database = currentDatabase() and table = '25400_dropped_tables'; |
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 also add a test that runs select * from system.dropped_tables_parts format Null
and works in parallel with other tests
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.
... format Null
will not test the issue with Ordinary - I added a test which will...
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.
Yes, but the purpose of this query is to find race conditions, not issues with Ordinary
After |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Table system.dropped_tables_parts contains parts of system.dropped_tables tables (dropped but not yet removed tables)
Documentation entry for user-facing changes