-
Notifications
You must be signed in to change notification settings - Fork 2.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
engine.Reload()
: read InnoDB tables sizes including FULLTEXT
index volume
#17118
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…w completely split the table identities from table sizes. Introducing baseShowInnodbTableSizes() Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…we're reading innodb table sizes; particular validation for nonzero filesize for partitioned table proves the logic is sound Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Working on the testing failures. We have pretty strict testing! |
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Converting to draft while working on all unit tests. |
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Good to review! All unit tests have been adapted (note: I'm impressed by just how much nitty gritty coverage they do for |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17118 +/- ##
==========================================
+ Coverage 67.15% 67.44% +0.28%
==========================================
Files 1571 1570 -1
Lines 252250 252285 +35
==========================================
+ Hits 169409 170148 +739
+ Misses 82841 82137 -704 ☔ View full report in Codecov by Sentry. |
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.
Nice fix to ensure we have the right information here.
Nice! This is looking great. |
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.
Nice work on this! Just had some minor nits and questions.
One larger thing that wasn't clear to me though... what role does TablesWithSize80 now play? Could/should we simplify that further so that it's not concerned with sizes at all?
@@ -366,6 +366,10 @@ func (*filePosFlavor) baseShowTablesWithSizes() string { | |||
return TablesWithSize56 | |||
} | |||
|
|||
func (filePosFlavor) baseShowInnodbTableSizes() string { | |||
return "" |
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.
Shouldn't this be:
mysqlFlavor{}.baseShowInnodbTableSizes()
? I think so...
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.
Just making sure I understand correctly, did you mean that filePosFlavor. baseShowInnodbTableSizes ()
should return the new query proposed in this PR, as opposed to an empty string? Why, then does filePosFlavor.baseShowTablesWithSizes()
return a TablesWithSize56
result as opposed to TablesWithSize80
?
In accordance with the rest of filePosFlavor
behavior, I don't think it should be using 8.0
-grade queries?
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 not sure. I don't think file/position is really tied to a version like that. We can leave it like it is.
@@ -22,6 +22,10 @@ func (mariadbFlavor) baseShowTables() string { | |||
return mysqlFlavor{}.baseShowTables() | |||
} | |||
|
|||
func (mariadbFlavor) baseShowInnodbTableSizes() string { | |||
return "" |
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 for this one, I think:
mysqlFlavor{}.baseShowInnodbTableSizes()
go/mysql/flavor_mysqlgr.go
Outdated
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.
mysqlgr (go/vt/vtgr
) is gone (#13308), but looks like we missed removing the flavor. Are you OK removing it here? We can also do that separately.
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 please do that separately!
// We therefore don't want to query for table sizes in getTableData() | ||
includeStats = false | ||
|
||
innodbResults, err := conn.Conn.Exec(ctx, innodbTableSizesQuery, maxTableCount, 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.
Not specific to this new work, but what does a Vitess user do if they have more than 10,000 tables? Seems like we do not support that today? I wonder if we need this limitation today (it was added 12 years ago, so while Vitess was only an internal YouTube project where exceeding that limit may have been unthinkable/impossible).
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.
Excellent question. Let's take it off this thread -- there's so much code in Vitess where we limit results to 10000 -- the good news is that it's relatively easy to find!
// innodbTableName is encoded any special characters are turned into some @0-f0-f0-f value. | ||
// Therefore this "#p#" here is a clear indication that we are looking at a partitioned table. |
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.
Nice! Confirmed that with the encoded names we don't have to worry about tables that actually have these special chars in them as this example is encoded as te@0023p@0023st
:
mysql> create table `te#p#st` (id int);
Query OK, 0 rows affected (0.01 sec)
mysql> SELECT it.name, its.file_size as normal_tables_sum_file_size, its.allocated_size as normal_tables_sum_allocated_size FROM information_schema.innodb_tables it JOIN information_schema.innodb_tablespaces its ON (its.space = it.space) WHERE its.name LIKE CONCAT(database(), '/%') AND its.name NOT LIKE CONCAT(database(), '/fts_%') UNION ALL SELECT it.name, SUM(its.file_size) as hidden_tables_sum_file_size, SUM(its.allocated_size) as hidden_tables_sum_allocated_size FROM information_schema.innodb_tables it JOIN information_schema.innodb_tablespaces its ON ( its.name LIKE CONCAT(database(), '/fts_', CONVERT(LPAD(HEX(table_id), 16, '0') USING utf8mb3) COLLATE utf8mb3_general_ci, '_%') ) WHERE its.name LIKE CONCAT(database(), '/fts_%') GROUP BY it.name;
+-----------------------------+-----------------------------+----------------------------------+
| name | normal_tables_sum_file_size | normal_tables_sum_allocated_size |
+-----------------------------+-----------------------------+----------------------------------+
| vt_commerce/product | 114688 | 114688 |
| vt_commerce/customer | 114688 | 114688 |
| vt_commerce/corder | 114688 | 114688 |
| vt_commerce/te@0023p@0023st | 114688 | 114688 |
| vt_commerce/members#p#p0 | 114688 | 114688 |
| vt_commerce/members#p#p1 | 114688 | 114688 |
| vt_commerce/members#p#p2 | 114688 | 114688 |
| vt_commerce/members#p#p3 | 114688 | 114688 |
| vt_commerce/members#p#p4 | 114688 | 114688 |
| vt_commerce/members#p#p5 | 114688 | 114688 |
+-----------------------------+-----------------------------+----------------------------------+
10 rows in set, 1 warning (0.01 sec)
Great question. It does not play any role at all and will never be used in 8.0. I should remove it altogether! |
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Eradicated |
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…dd 'Legacy' tests in engine_test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Addendum: this PR introduces a logic difference between MySQL 8.0 and legacy versions (5.7). The code path for As such, we notice our unit testing fakes 25949d4 introduces the ability to set up a "legacy" test environment, and splits/duplicates some unit testing for the different environments: (
See this commit for the relevant changes. See also #17168 |
Description
Followup to #17066, and see #17119.
The changes in #17066 solve a performance issue. We note, however, that the query
TablesWithSize80
does not include the file size / allocated size for InnoDBFULLTEXT
indexes.InnoDB's
FULLTEXT
index isn't a BTREE, and is implemented using hidden InnoDB tables. Those are visible ininformation_schema.innodb_tablespaces
. However, in that table they are not named after the original table. Instead, they look something likemydb/fts_000000000000075e_00000000000005f9_index_2
.The key to unlocking these table identities is in
information_schema.innodb_tables
, which introduces this table as a third table in theTablesWithSize80
, and this introduces two problems:innodb_table
names are encoded. The matchingmy-db/my-table
entry ininnodb_tablespaces
will look likemy@002ddb/my@002dtable
ininnodb_tables
. This makes it impossible to join between the three tables. (Note: MySQL has afilename
charset that can translate the two. But it was made internal only and inaccessible to queries).In between the two problems, this PR takes a completely different approach on MySQL 8.0:
innodb_tables
andinnodb_tablespaces
using valid conditions (which are not the tablename
).FULLTEXT
keys)schema.Engine
reads table data without sizes, reads the InnoDB table sizes as explained, then programmatically joins the two.On non MySQL 8.0 the behavior remains unchanged.
This PR should pass all existing tests. We also add a unit test designed for
schema.Engine.Reload()
that validates the operation of the new logic.Related Issue(s)
schema.Engine
: need to improve on tables-with-sizes query #17119schema.Engine
#17168Checklist
Deployment Notes