Do not use THR_LOCK for MyRocks - Githubissues

facebook / mysql-5.6

Facebook's branch of the Oracle MySQL database. This includes MyRocks.

http://myrocks.io

Other

2.48k stars 714 forks source link

Do not use THR_LOCK for MyRocks #1323

Closed laurynas-biveinis closed 1 year ago

laurynas-biveinis commented 1 year ago

Since 5.7, all the THR_LOCK uses cases for InnoDB are fully covered by MDL locking and thus InnoDB removed THR_LOCK-based locking. The same MDL lock coverage applies to MyRocks as well, thus remove THR_LOCK-based locking there too.

Remove fields Rdb_table_handler::m_thr_lock and ha_rocksdb::m_db_lock
Add HA_NO_READ_LOCAL_LOCK to ha_rocksdb::table_flags to indicate that the SE does not have any specific support for LOCK TABLE READ LOCAL
New overridden method ha_rocksdb::lock_count that always returns zero
Remove all code in ha_rocksdb::store_lock that created THR_LOCK objects. Leave only the code that decides on the internal RocksDB locking.

References:

https://dev.mysql.com/worklog/task/?id=6671
https://dev.mysql.com/doc/relnotes/mysql/5.7/en/news-5-7-5.html
5c6171e2536b3274044354471e1ace9c878b8aa4

facebook-github-bot commented 1 year ago

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

facebook-github-bot commented 1 year ago

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

facebook-github-bot commented 1 year ago

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

facebook-github-bot commented 1 year ago

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

laurynas-biveinis commented 1 year ago

Rebased and retested with no changes

facebook-github-bot commented 1 year ago

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

luqun commented 1 year ago

Please rebase this PR.

facebook-github-bot commented 1 year ago

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

laurynas-biveinis commented 1 year ago

@luqun , rebased

facebook-github-bot commented 1 year ago

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

laurynas-biveinis commented 1 year ago

This has a minor effect for DD: by making MyRocks behave like InnoDB WRT locking, at least one more existing testcase can be run as-is under both DDSEs (main.status)

luqun commented 1 year ago

This has a minor effect for DD: by making MyRocks behave like InnoDB WRT locking, at least one more existing testcase can be run as-is under both DDSEs (main.status)

Currently, do you try to run main test suite under myrocks dd and find out main.status need this change?

laurynas-biveinis commented 1 year ago

This has a minor effect for DD: by making MyRocks behave like InnoDB WRT locking, at least one more existing testcase can be run as-is under both DDSEs (main.status)

Currently, do you try to run main test suite under myrocks dd and find out main.status need this change?

Yes, but that was not the motivation to do it. If this PR did not exist, I'd just skip the testcase