Improve Schema Engine's TablesWithSize80 query

[mattlord]

Description

With only the query change the manual test in #17059 passes. The reason that this impacts the OnlineDDL cutover is because:

1. We have thousands of tables
2. This made our I_S query that the tabletserver's Schema Engine uses to maintain a cache of the table schemas very slow
3. The DDL that we do is done via the ApplySchema RPC, which tells the schema engine to reload its cache at the end, which uses the noted slow query in step 2. This causes the Schema Engine's cache to be reloaded while holding the mutex.
4. OnlineDDL creates a sentry table (possibly leading to another Schema Engine reload) just before waiting for VReplication to catch up
5. VReplication tries to process the next GTID and get the new sentry table’s columns from the Schema Engine cache when processing the TABLE_MAP event in order to build and cache the table plan, and blocks on eventually reading from the cache because the mutex is held in step 3
6. VReplication cannot get the columns for the table in order to map the row event to the table's columns before the 10 second (by default) timeout that OnlineDDL uses when waiting for VReplication to catch up (default)

So because the modified query is ~ 260x faster than the original — with the original taking about 13000ms and the new one about 80ms (see below) — we are able to update the schema cache and read the current tables schema for vreplication within the relatively default short timeout that OnlineDDL uses when waiting for vreplication to reach the position where the primary was.

Using the manual test from the issue again, we can compare the query times:

command mysql -u root --socket ${VTDATAROOT}/vt_0000000$(vtctldclient GetTablets --keyspace commerce --tablet-type primary --shard "0" | awk '{print $1}' | cut -d- -f2 | bc)/mysql.sock vt_commerce 

mysql> SELECT t.table_name, t.table_type, UNIX_TIMESTAMP(t.create_time), t.table_comment, SUM(i.file_size), SUM(i.allocated_size) FROM information_schema.tables t LEFT JOIN information_schema.innodb_tablespaces i ON i.name LIKE CONCAT(t.table_schema, '/', t.table_name, IF(t.create_options <=> 'partitioned', '#p#%', '')) COLLATE utf8mb3_general_ci WHERE t.table_schema = database() GROUP BY t.table_schema, t.table_name, t.table_type, t.create_time, t.table_comment;
...
3004 rows in set (13.43 sec)

mysql> SELECT t.table_name,     t.table_type,     UNIX_TIMESTAMP(t.create_time),     t.table_comment,     SUM(i.file_size),     SUM(i.allocated_size)   FROM information_schema.tables t   LEFT JOIN (SELECT name, file_size, allocated_size FROM information_schema.innodb_tablespaces WHERE name LIKE CONCAT(database(), '/%')) i   ON i.name = CONCAT(t.table_schema, '/', t.table_name) COLLATE utf8mb3_general_ci   WHERE     t.table_schema = database() AND IFNULL(t.create_options, '') NOT LIKE '%partitioned%'   GROUP BY     t.table_schema, t.table_name, t.table_type, t.create_time, t.table_comment UNION ALL SELECT t.table_name,     t.table_type,     UNIX_TIMESTAMP(t.create_time),     t.table_comment,     SUM(i.file_size),     SUM(i.allocated_size)   FROM information_schema.tables t   LEFT JOIN (SELECT name, file_size, allocated_size FROM information_schema.innodb_tablespaces WHERE name LIKE CONCAT(database(), '/%')) i     ON i.name LIKE (CONCAT(t.table_schema, '/', t.table_name, '#p#%') COLLATE utf8mb3_general_ci)   WHERE     t.table_schema = database() AND t.create_options LIKE '%partitioned%'   GROUP BY     t.table_schema, t.table_name, t.table_type, t.create_time, t.table_comment;
...
3004 rows in set (0.05 sec)

With only the timeout related changes the manual test in #17059 ALSO passes. The reason being that we're able to get past our schema reload work and reach the desired position within the larger timeout (the query blocking us takes ~ 13 seconds).

So we have two fixes for the underlying issue that not only solves the OnlineDDL specific aspects but also makes Vitess much more efficient overall when there are thousands of tables.

Related Issue(s)

• Fixes: Bug Report: OnlineDDL migrations fail when keyspace has thousands of tables #17059

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 67.17%. Comparing base (4e385ce) to head (8df9040).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vttablet/onlineddl/executor.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17066      +/-   ##
==========================================
+ Coverage   67.16%   67.17%   +0.01%     
==========================================
  Files        1571     1571              
  Lines      252097   252250     +153     
==========================================
+ Hits       169318   169457     +139     
- Misses      82779    82793      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dbussink]

--- FAIL: TestDropViewDDL (62.05s)
    views_test.go:189: all views are not dropped within the time

Do the query changes here now work correctly with views? There's a failing end-to-end test now.

[shlomi-noach]

Yeah, the new query does not work for views. Looking into.

[shlomi-noach]

Good old CREATE_OPTIONS IS NULL for views, which is why the original query had <=>. OK, easy enough to fix.

[shlomi-noach]

Support for views (NULL for TABLE_OPTIONS) in 0467caa

[harshit-gangal]

these comments were helpful.
Can you add the comments with new changes to know why it is done this way.

[shlomi-noach]

Added useful comments :)

[shlomi-noach]

@arthurschreiber the current change goes back to two SELECT queries combined with UNION ALL. In #15713 you had reservations about using two queries, but right now this looks to be the most optimal solution that is also correct. Would you care to try the revised query in your environment to see how it performs?

[arthurschreiber]

I'm taking a look right now.

[arthurschreiber]

So I took a look at #15713, and I'm not even sure the change in that PR was good.

Testing on my system, the original query takes 0.4 seconds, and the query proposed in #15713 takes 17 seconds (!).

The new query here brings query execution time down to 0.4 seconds.

One improvement I could find was to change the innodb_tablespaces join slightly:

SELECT t.table_name,
    t.table_type,
    UNIX_TIMESTAMP(t.create_time),
    t.table_comment,
    SUM(i.file_size),
    SUM(i.allocated_size)
  FROM information_schema.tables t
  LEFT JOIN (SELECT name, file_size, allocated_size FROM information_schema.innodb_tablespaces WHERE name LIKE CONCAT(database(), '/%')) i
  ON i.name = CONCAT(t.table_schema, '/', t.table_name) COLLATE utf8mb3_general_ci
  WHERE
    t.table_schema = database() AND IFNULL(t.create_options, '') NOT LIKE '%partitioned%'
  GROUP BY
    t.table_schema, t.table_name, t.table_type, t.create_time, t.table_comment
UNION ALL
SELECT t.table_name,
    t.table_type,
    UNIX_TIMESTAMP(t.create_time),
    t.table_comment,
    SUM(i.file_size),
    SUM(i.allocated_size)
  FROM information_schema.tables t
  LEFT JOIN (SELECT name, file_size, allocated_size FROM information_schema.innodb_tablespaces WHERE name LIKE CONCAT(database(), '/%')) i
    ON i.name LIKE (CONCAT(t.table_schema, '/', t.table_name, '#p#%') COLLATE utf8mb3_general_ci)
  WHERE
    t.table_schema = database() AND t.create_options LIKE '%partitioned%'
  GROUP BY
    t.table_schema, t.table_name, t.table_type, t.create_time, t.table_comment

That brings the query down to 0.37 seconds.

I tried multiple things to merge the two queries to reduce the accesses to information_schema.innodb_tablespaces down to 1, but they all performed worse than this.

[shlomi-noach]

@arthurschreiber thank you for taking a look!

Yeah, I, too, tried the LEFT JOIN (SELECT name, file_size, allocated_size FROM information_schema.innodb_tablespaces WHERE name LIKE CONCAT(database(), '/%')) i variation locally and could not reliably identify a change either way. There is the potential for MySQL to create an indexed temporary table either with or without this change. I think the difference between the two variations is small enough that it is not worth the added complexity, but I won't die on this hill.

Glad to hear the query in this PR works well for you!

[arthurschreiber]

@shlomi-noach I see a consistent 10% improvement, so it would be nice to have that change. In our testing system, we start a lot of keyspaces in a single vtcombo instance, and I remember this query being one of the places where we end up spending quite a bit of time in.

[arthurschreiber]

Either way, this is a big improvement compared to before! ❤️

[shlomi-noach]

@arthurschreiber the query now uses the subselect clause.