search

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

Fix handling race conditions on statistics writes when DDSE is MyRocks #1363

Closed laurynas-biveinis closed 1 year ago

laurynas-biveinis commented 1 year ago

If two threads attempt to update statistics for the same object in parallel, one of them may fail, and the failure is discarded. InnoDB fails with ER_DUP_ENTRY, but MyRocks may return ER_LOCK_WAIT_TIMEOUT instead in the same situation, thus handle accordingly. The server layer patch could be avoided by making MyRocks return ER_DUP_ENTRY too in the same situation, but I don't see an obvious way to do it.

This fixes main.information_schema_statistics and main.alter_table_myisam when DDSE == MyRocks

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

https://github.com/facebook/mysql-5.6/pull/1363 @luqun, @hermanlee, we discussed offline that enabling RocksDB deadlock detection would allow not patching the server layer. But after investigating, it does not look that deadlock detection is related. The problem is thread 1 inserting a row to a stat table and thread 2 preparing to insert or update a row with the same primary key (GetForUpdate). There is no deadlock because thread 1 is not waiting for any locks held by thread 2, and InnoDB is returning ER_DUP_ENTRY and not ER_LOCK_DEADLOCK in this case.

If deadlock detection is excluded, then in order not to patch the server layer MyRocks would need to return ER_DUP_ENTRY. Currently it does so only for bulk loads. The comments for GetForUpdate do not suggest the returned errors are granular enough to tell the ER_DUP_ENTRY case apart from the other ones.

luqun commented 1 year ago

pushed..