Skip to content
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

bugfix: ensure select for update retry correctly #1452

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@ggndnn
Copy link
Contributor

commented Aug 13, 2019

Ⅰ. Describe what this PR did

In SelectForUpdateExecutor, select-for-update statement is not in global locking retry loop, so the data read is not necessarily globally committed. This PR ensue select-for-update retry correctly.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io

This comment has been minimized.

Copy link

commented Aug 13, 2019

Codecov Report

Merging #1452 into develop will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1452      +/-   ##
=============================================
- Coverage      45.69%   45.67%   -0.02%     
  Complexity      1664     1664              
=============================================
  Files            345      345              
  Lines          12673    12673              
  Branches        1593     1593              
=============================================
- Hits            5791     5789       -2     
- Misses          6234     6235       +1     
- Partials         648      649       +1
Impacted Files Coverage Δ Complexity Δ
...ta/rm/datasource/exec/SelectForUpdateExecutor.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...very/registry/zk/ZookeeperRegisterServiceImpl.java 63.77% <0%> (-0.79%) 24% <0%> (ø)
...server/store/file/FileTransactionStoreManager.java 45.99% <0%> (-0.35%) 19% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0159009...8959c20. Read the comment docs.

@slievrly

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@ggndnn good found. Does it work if the db's isolation level is set to repeatable read?

@ggndnn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@slievrly I think it works. Because select-for-update statement returns current latest data in db, event set to repeatable read level, not like normal select statement who may query data from snapshot.

@slievrly

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@ggndnn Before the code is modified: select for update In the local transaction, if it succeeds, it will hold the X lock. It does not release the X lock. Who can modify the data?

@ggndnn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@slievrly Though X lock is hold, but it is released by conn.rollback when LockConflictException. Meanwhile some other tx may modify the data.

catch (LockConflictException lce) {
    conn.rollback(sp);
    lockRetryController.sleep(lce);
}
@slievrly

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

catch (LockConflictException lce) {
   conn.rollback(sp);
   lockRetryController.sleep(lce);
}

So, I think conn.rollback(sp) is executed when the last retry is unsuccessful, instead of going to rollback every time.

@slievrly

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@ggndnn After considering I think your plan is more appropriate. Because if I follow my plan, I got the X lock of the database and didn't get the global lock. At the same time, another distributed transaction gets the global lock and releases the database lock. When it wants to perform the globalRollback, it will cause deadlock.

Copy link
Member

left a comment

LGTM

@ggndnn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@slievrly Thanks for your review.
One question, I did not find global lock here, only database lock, any suggestion? I think conn.rollback(sp) is executed every time when checkLock throws LockConflictException. Is my understanding correct?

@ggndnn ggndnn force-pushed the ggndnn:bugfix_ensure_select_for_update_retry branch from b45c037 to a8cd02e Aug 14, 2019
@slievrly

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@slievrly Thanks for your review.
One question, I did not find global lock here, only database lock, any suggestion? I think conn.rollback(sp) is executed every time when checkLock throws LockConflictException. Is my understanding correct?

According to the currently modified code, this understanding is correct. The current select for update only checks that the lock does not occupy the lock. In the future, it is necessary to do a lock strategy to keep the behavior consistent with the local transaction.

Copy link
Contributor

left a comment

okey to me

@slievrly slievrly merged commit 73cd6d3 into seata:develop Aug 14, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@VainEffort VainEffort referenced this pull request Sep 10, 2019
0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.