search

facebook / mysql-5.6

Facebook's branch of the Oracle MySQL database. This includes MyRocks.
http://myrocks.io
Other
2.47k stars 711 forks source link

Make MyRocks DD commits always durable #1403

Closed laurynas-biveinis closed 6 months ago

laurynas-biveinis commented 7 months ago

Regular commit durability is governed by the binlog group commit algorithm and system variables. Make any commits (and prepares in 2PC) involving DD tables always durable unconditionally, just like InnoDB does. It is implemented through a new flag Rdb_transaction::m_dd_transaction, which is set in ha_rocksdb::external_lock and checked in rocksdb_prepare. To avoid confusion, rename the existing m_ddl_transaction flag to m_bulk_index_transaction.

facebook-github-bot commented 7 months ago

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

luqun commented 7 months ago

how about check thd_is_dd_update_stmt() during trx prepare(or m_ddl_transaction = thd_is_dd_update_stmt(thd))? so that we have similar behavior as innodb?

such as 

  if (thd_is_dd_update_stmt(thd)) {
    tx->set_sync(true);

    or 

    m_ddl_transaction = thd_is_dd_update_stmt(thd);

    Related innodb code: https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.32/storage/innobase/trx/trx0trx.cc#L2034
laurynas-biveinis commented 6 months ago

how about check thd_is_dd_update_stmt() during trx prepare(or m_ddl_transaction = thd_is_dd_update_stmt(thd))?

Unfortunately thd_is_dd_update_stmt is true in fewer instances than ha_rocksdb::external_lock(F_WRLCK) is called on a table with a DD category and making this change re-introduces crash recovery test failures. For InnoDB, thd_is_dd_update_stmt is also not the only input for trx->ddl_operation.

Some examples of queries where the two differ, test rocksdb_dd_innodb.ddl_crash_basic:

Mismatch in external_lock: table_share->table_category == TABLE_CATEGORY_DICTIONARY = 1, thd_is_dd_update_stmt() = 1, query = OPTIMIZE TABLE t1
Mismatch in rocksdb_prepare: is_dd_update = 0, tx->is_dd_transaction() = 1, query = OPTIMIZE TABLE t1
Mismatch in external_lock: table_share->table_category == TABLE_CATEGORY_DICTIONARY = 1, thd_is_dd_update_stmt() = 1, query = DROP TABLE t1
Mismatch in rocksdb_prepare: is_dd_update = 0, tx->is_dd_transaction() = 1, query = DROP TABLE t1
Mismatch in external_lock: table_share->table_category == TABLE_CATEGORY_DICTIONARY = 1, thd_is_dd_update_stmt() = 1, query = CREATE TABLESPACE ts1 ADD DATAFILE 'ts1.ibd'
Mismatch in rocksdb_prepare: is_dd_update = 0, tx->is_dd_transaction() = 1, query = CREATE TABLESPACE ts1 ADD DATAFILE 'ts1.ibd'
Mismatch in external_lock: table_share->table_category == TABLE_CATEGORY_DICTIONARY = 1, thd_is_dd_update_stmt() = 1, query = ALTER TABLESPACE ts1 RENAME TO ts1_renamed
Mismatch in external_lock: table_share->table_category == TABLE_CATEGORY_DICTIONARY = 1, thd_is_dd_update_stmt() = 1, query = ALTER TABLESPACE ts1 RENAME TO ts1_renamed
Mismatch in rocksdb_prepare: is_dd_update = 0, tx->is_dd_transaction() = 1, query = ALTER TABLESPACE ts1 RENAME TO ts1_renamed
laurynas-biveinis commented 6 months ago

There are also some cases where thd_is_dd_update_stmt is set, but no DD table is write-locked in MyRocks: CREATE EVENT event_t1 ... which accesses a system, not a DD table.

facebook-github-bot commented 6 months ago

@laurynas-biveinis has updated the pull request. You must reimport the pull request before landing.

laurynas-biveinis commented 6 months ago

@luqun, rebased with no substantial changes (for now?), added a small tweak to ha_rocksdb::is_dd_update not to check default_dd_system_storage_engine.

facebook-github-bot commented 6 months ago

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