Upgrade all system tables(exclude mysql.gtid_executed) from INNODB to MyRocks

[luqun]

Summary:

During mysqld version changes, it call upgrade_system_schemas() to upgrade SYSTEM tables. During DDSE change, It will also call upgrade_system_schemas() to upgrade these SYSTEM tables.

One question is how to "reliability" upgrade SYSTEM tables during DDSE change(upgrade SYSTEM tables occurs after DD_ENGINE properties has been changed to target DDSE):

• [Current] add a new persistent variable DD_ENGINE_UPGRADED in mysql.dd_properities table, whose value is last dd engine name during dd engine change. For example, when DDSE is changed from INNODB to ROCKSDB, the DD_ENGINE_UPGRADED value will be set to "INNODB" during DD Tables upgrade. Later, mysqld will check whether DD_ENGINE_UPGRADED is same as target DDSE in no_server_upgrade_required(), if not, upgrade these SYSTEM tables to target DDSE if required.
• add a new global variables, such as DD_ENGINE_UPGRADING. During crash recovery, we need to re-initiate this values.

Details:

• add a new Properties DD_ENGINE_UPGRADED into DD_Properties table
• mysql_system_tables_fix.sql: update system tables if necessary.
• main.dd_schema_definition_after_upgrade_57_debug, main.dd_schema_definition_after_upgrade_80_debug: skip DD_ENGINE_UPGRADED compare, similar to DD_ENGINE
• rocksdb.rocksdb_read_free_rpl, rocksdb.singledelete: add delta for rocksdb stats due to system table involved during commit
• rocksdb.cancel_mc, rocksdb.rocksdb_fault_injection: force to use INNODB DDSE

The change has several assumptions:

• SYSTEM Tables and DD Tables always has same SE(DDSE).
• When force upgrade is specified(https://dev.mysql.com/doc/refman/8.0/en/upgrading-what-is-upgraded.html), it won't run ALTER SYSTEM tables to target DDSE again and it will run similar as mysqld version change

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: https://phabricator.intern.facebook.com/D51085626

[luqun]

Can you explain why exactly is the new DD_PROPERTIES entry required? Would the following work without it?

• Handle DD tables first, as usual
• Go through all the system tables, if engine != dd engine, do ALTER TABLE

To me this looks crash-safe too, unless I'm missing something.

Yes, it is crash-safe and also quick restart. For quick restart, in order to detect whether we need to run alter tables during restart, either check all SYSTEM tables or add a mark to record state

[laurynas-biveinis]

Yes, it is crash-safe and also quick restart. For quick restart, in order to detect whether we need to run alter tables during restart, either check all SYSTEM tables or add a mark to record state

Checking this property for SYSTEM tables should be cheap (they will be in the DD cache). I am a bit worried that a new DD_PROPERTIES will complicate things (i.e. have to delete it on MyRocks -> InnoDB DDSE change), but if you prefer this approach, it's OK.

[luqun]

(i.e. have to delete it on MyRocks -> InnoDB DDSE change)

I think we can keep these variables even for MyRocks->INNODB DDSE change. I agree that remove these variables will be tricky during that DDSE change.

[luqun]

@laurynas-biveinis , would you take a look again? just rebase with your latest change. also use script to upgrade these system tables.

[laurynas-biveinis]

I believe we have to fully think through user-issued ALTER TABLE mysql.systable ENGINE=anything support.

With your approach, or with previously-suggested my one, or, I believe, with any approach in the server, any ALTER TABLE that changes engine away from the default_dd_system_storage_engine will be either ignored (good for user who issued it but bad for the idea behind this PR), either reverted (good for this PR, bad for the user).

I see two ways out of this:

• In the server, document that default_dd_system_storage_engine applies to new instances only, handle any InnoDB <-> MyRocks changing at user level, effectively dropping this PR
• In the server, block ALTER TABLE mysql.* ENGINE = not-default-dd-system-one. This should be done in this PR too then.

What do you think?

[luqun]

What do you think?

Discuss offline and we believe we should disable "alter table system tables into non-DDSE" for users to keep simple. All DD Tables and SYSTEMS Tables always use DDSE.

[luqun]

@laurynas-biveinis , now it is ready for review.. :)

[luqun]

Resolved all comments and also add a new variables skip-sys-tables-engine-check, since some MTRs alter system table to myiasm engine.

[laurynas-biveinis]

also add a new variables skip-sys-tables-engine-check, since some MTRs alter system table to myiasm engine.

Since we will never be changing system table engine to MyISAM, can we just disable those testcases instead?

[luqun]

also add a new variables skip-sys-tables-engine-check, since some MTRs alter system table to myiasm engine.

Since we will never be changing system table engine to MyISAM, can we just disable those testcases instead?

Problem is that these testcase also test other scenarios in same time..maybe a new global variable is better..

[laurynas-biveinis]

luqun @.***> writes:

yep. it complains Static_cast from 'legacy_db_type ' to 'uint ' (aka 'unsigned int *') is not allowed.

Right, it's pointers. OK to leave as-is.

[laurynas-biveinis]

luqun @.***> writes:

also add a new variables skip-sys-tables-engine-check, since some MTRs alter system table to myiasm engine.

Since we will never be changing system table engine to MyISAM, can we just disable those testcases instead?

Problem is that these testcase also test other scenarios in same time..maybe a new global variable is better..

Can you list those tests?

If they happen to be debug-only, then we can piggyback on +d,skip_dd_table_access_check. The debug injection name is not a perfect match, but it would suffice IMHO.

[luqun]

Can you list those tests? If they happen to be debug-only, then we can piggyback on +d,skip_dd_table_access_check. The debug injection name is not a perfect match, but it would suffice IMHO. There is only 1 debug only test..

mysql-test/suite/perfschema/t/error_log.test mysql-test/t/system_tables_myisam.test mysql-test/t/system_tables_myisam_lctn_1.test mysql-test/t/transactional_acl_tables.test mysql-test/suite/auth_sec/t/atomic_rename_user.test mysql-test/t/dd_schema_dd_properties_debug.test

[laurynas-biveinis]

mysql-test/suite/perfschema/t/error_log.test mysql-test/t/system_tables_myisam.test mysql-test/t/system_tables_myisam_lctn_1.test mysql-test/t/transactional_acl_tables.test mysql-test/suite/auth_sec/t/atomic_rename_user.test mysql-test/t/dd_schema_dd_properties_debug.test

I see. It would require some effort to split these tests into MyISAM and non-MyISAM parts, feel free to go ahead with the new sysvar or split the tests as you see best

[facebook-github-bot]

@luqun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

This pull request has been merged in facebook/mysql-5.6@f73d8f48babd49778e588bf88f2e3647c22776d4.