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

Implement MyRocks DDSE handler & handlerton methods #1333

Closed laurynas-biveinis closed 12 months ago

laurynas-biveinis commented 1 year ago

Implement MyRocks DDSE handler & handlerton methods

laurynas-biveinis commented 1 year ago

The pull request is ready for review but merging it will not result in a viable MyRocks DDSE (and will in fact break the existing DDSE = MyRocks testcases). All the code has been tested in another branch. The missing prerequisites for viable DDSE are

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

Call it from create_key_defs for DD tables to get the pre-assigned index IDs in advance, as opposed to user tables where the index IDs are assigned later.

Why do DD tables need to get pre-assigned index IDs? could it also do similar as user tables? If I understand correctly, with myrocks private DD, DD tables can also get index IDs are assigned later. after removing myrocks private DD, then we need to get pre-assigned index IDs.

laurynas-biveinis commented 1 year ago

Call it from create_key_defs for DD tables to get the pre-assigned index IDs in advance, as opposed to user tables where the index IDs are assigned later.

Why do DD tables need to get pre-assigned index IDs? could it also do similar as user tables? If I understand correctly, with myrocks private DD, DD tables can also get index IDs are assigned later. after removing myrocks private DD, then we need to get pre-assigned index IDs.

The MySQL DD layer calls the pre-assignment code. I guess we could try making it a no-op and postpone the assignment until the handler create_table call, where the assignment could be re-unified with user table assignment.

luqun commented 1 year ago

Could you rebase this PR?

luqun commented 1 year ago

I guess we could try making it a no-op and postpone the assignment until the handler create_table call, where the assignment could be re-unified with user table assignment.

Yes. If we postpone the assignment, so all rocksdb table assignment will be unified..

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. I will revisit the ID assignment after finishing my current task. One point requiring consideration there is that the DD/SE layer has the notion of "reset", making the IDs restart, which the MyRocks DD does not do. Yet, somehow the current code does not desync the IDs even though the server DD layer resets and the MyRocks DD does not.

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

@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

@luqun, I have looked into removing the private index ID assignment from ha_rocksdb::get_se_private_data and leaving it to the MyRocks DD, as it currently does for the non-DD tables. In this approach, the MyRocks DD layer would have to return the assigned ID to the server layer, because all DD tables are expected to have their IDs assigned. This should be done by writing them to ha_rocksdb::create argument dd::Table *table_def. But there is a catch: in order for the DD layer to take any table_def modifications into account, the handlerton must be declared with HTON_SUPPORTS_ATOMIC_DDL flag (I guess because those table_def changes become DD updates in a transaction, and if the DDL statement is aborted, they have to be rolled back together with any SE-made changes).

MyRocks is currently not a HTON_SUPPORTS_ATOMIC_DDL engine. If/when it becomes one, we can revisit, but for the time being I'll cleanup this PR using the current design.

laurynas-biveinis commented 1 year ago

@luqun ready for review! These are close to final versions of handler/handlertons. A few TODOs in the code mostly related to the upgrade code paths.

No functional changes with InnoDB DDSE, safe to merge.

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

@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
luqun commented 1 year ago

the MyRocks DD layer would have to return the assigned ID to the server layer, because all DD tables are expected to have their IDs assigned.

@laurynas-biveinis , Change LGTM. just curious, for "all DD tables are expected to have their IDs assigned", these IDs are se private id instead DD layer IDs. if myrocks doesn't return and also didn't check these IDs, then myrocks should be okay, is it?

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

the MyRocks DD layer would have to return the assigned ID to the server layer, because all DD tables are expected to have their IDs assigned.

@laurynas-biveinis , Change LGTM. just curious, for "all DD tables are expected to have their IDs assigned", these IDs are se private id instead DD layer IDs. if myrocks doesn't return and also didn't check these IDs, then myrocks should be okay, is it?

@luqun, I had implemented that, the problem was that the server DD layer expects this property to be set

laurynas-biveinis commented 11 months ago

@luqun, the last changes to this PR were lost in the trunk push. Can you fix this or should I make a PR for the delta?