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

Support mysql.db system table in MyRocks engine #1407

Closed laurynas-biveinis closed 5 months ago

laurynas-biveinis commented 6 months ago
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.

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.

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.

luqun commented 6 months ago

Thanks for adding WL#14087. do you happen to know why innodb doesn't need is_acl_table(table)? such as

if (m_prebuilt->table->is_dd_table || m_prebuilt->no_read_locking || is_acl_table(table)) {

https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.32/storage/innobase/handler/ha_innodb.cc#L19054

laurynas-biveinis commented 6 months ago

do you happen to know why innodb doesn't need is_acl_table(table)? such as

if (m_prebuilt->table->is_dd_table || m_prebuilt->no_read_locking || is_acl_table(table)) {

https://github.com/facebook/mysql-5.6/blob/fb-mysql-8.0.32/storage/innobase/handler/ha_innodb.cc#L19054

ACL tables get extra(HA_EXTRA_NO_READ_LOCKING) called beforehand, this gets checked in the assert right above those lines: ut_ad(!m_prebuilt->no_read_locking || m_prebuilt->table->is_dd_table || is_acl_table(table));

I believe MyRocks logic here is an exact copy but written differently, MyRocks does not do the equivalent of is_acl_table at this point neither, except in an assert

facebook-github-bot commented 5 months ago

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

laurynas-biveinis commented 5 months ago

Rebased, addressed the first comment, the 2nd comment still open

facebook-github-bot commented 5 months ago

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

laurynas-biveinis commented 5 months ago

@luqun , updated with index ID masking, ready for review

facebook-github-bot commented 5 months ago

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

facebook-github-bot commented 5 months ago

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