-
Notifications
You must be signed in to change notification settings - Fork 717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #802: MyRocks: Statement rollback doesnt work correctly for nes… #804
Issue #802: MyRocks: Statement rollback doesnt work correctly for nes… #804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
storage/rocksdb/ha_rocksdb.cc
Outdated
@@ -2449,6 +2453,10 @@ class Rdb_transaction_impl : public Rdb_transaction { | |||
rocksdb::Transaction *m_rocksdb_tx = nullptr; | |||
rocksdb::Transaction *m_rocksdb_reuse_tx = nullptr; | |||
|
|||
// Number of savepoints to roll back to get to the start of the | |||
// "top-level" statement. | |||
int m_n_savepoints; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably make more sense to have these variables and collapse_savepoint() call in Rdb_transaction instead?
… for nested statements Variant#2: Use a different approach to taking RocksDB SavePoints. - Take one at transaction start (so we can rollback to it) - Take one when top-level statement completes (there is a place to do that. Detecting when a top-level statement starts is much harder) - Also, don't make a new Savepoint if the previous statement didn't make any writes. This way, a long transaction doing lots of SELECTs will not create a lot of savepoints. - Also, don't call get_or_create_tx() from ha_rocksdb::external_lock(F_UNLCK). This call is made right after rocksdb_commit(). We end up creating a new RocksDB transaction just to do cleanups and immediately discard it.
f75f876
to
612ec75
Compare
@spetrunia has updated the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@@ -1854,6 +1854,17 @@ class Rdb_transaction { | |||
bool m_is_delayed_snapshot = false; | |||
bool m_is_two_phase = false; | |||
|
|||
private: | |||
/* Number of RockDB savepoints taken */ | |||
int m_n_savepoints; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_n_savepoints and m_writes_at_last_savepoint need to be initialized at similar locations to m_write_count, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Lost that initialization when re-working the patch. Will now add it back
… for nested statements Variant facebook#2: initialize m_n_savepoints and m_writes_at_last_savepoint at transaction start.
@spetrunia has updated the pull request. View: changes, changes since last import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hermanlee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hermanlee is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Cherry-pick this fix from the upstream: commit 6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 Author: Sergei Petrunia <psergey@askmonty.org> Date: Tue Apr 10 11:43:01 2018 -0700 [mysql-5.6][PR] Issue #802: MyRocks: Statement rollback doesnt work correctly for nes� Summary: �ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod-------- Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod201801 Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod201801 Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod201801 Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
… work correctly for nes… Upstream commit ID : fb-mysql-5.6.35/6ddedd8f1e0ddcbc24e8f9a005636c5463799ab7 PS-4476 : Merge prod201801 Summary: …ted statements Variant percona#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 9a6f414
…t work correctly for nes… Summary: …ted statements Variant facebook#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 GitHub Author: Sergei Petrunia <psergey@askmonty.org> Test Plan: Imported from GitHub, without a `Test Plan:` line. Reviewers: svcscm, generatedunixname499836121 Reviewed By: svcscm Subscribers: phabricatorlinter, webscalesql-eng@fb.com Differential Revision: https://phabricator.intern.facebook.com/D7509380 Signature: 7509380:1523385571:df3dca173fe59bcf217d3a77f79eb085980149fc
… for nes… (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 979fc37
… for nes… (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 979fc37
… for nes… (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 979fc37
… for nes… (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 979fc37
… for nes… (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee fbshipit-source-id: 979fc37
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(#804)?= (#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes #804 Differential Revision: D7509380 (6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 (facebook@6ddedd8) Pulled By: hermanlee fbshipit-source-id: be411bfae1b
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(percona#804)?= (percona#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant facebook#1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(facebook#804)?= (facebook#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook#804 Differential Revision: D7509380 Pulled By: hermanlee
…?=20doesnt=20work=20correctly=20for=20nes=E2=80=A6=20(percona#804)?= (percona#804) Summary: …ted statements Variant #1: When the statement fails, we should roll back to the latest savepoint taken at the top level. Closes facebook/mysql-5.6#804 Differential Revision: D7509380 Pulled By: hermanlee
…ted statements
Variant #1: When the statement fails, we should roll back to the latest
savepoint taken at the top level.