bugfix: ensure select for update retry correctly

[ggndnn]

Ⅰ. 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]

Codecov Report

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

@@              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]

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

[ggndnn]

@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]

@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]

@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]

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]

@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.

[slievrly]

LGTM

[ggndnn]

@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?

[slievrly]

@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.

[sharajava]

okey to me