*: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() #33947

morgo · 2022-04-13T15:37:51Z

What problem does this PR solve?

Issue Number: close #14994

Problem Summary:

This supports the GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() functions previously proposed in the design document @ #33406

What is changed and how it works?

Functions GET_LOCK, RELEASE_LOCK, RELEASE_ALL_LOCKS are no longer noops. The functionality is now implemented.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
Deployed 2x tidb, 1x tidb/pd. Confirmed functionality works correctly.
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Affects user behaviors
Contains syntax changes
Contains variable changes
Changes the behavior of tidb_enable_noop_funcs, so documentation needs updating.
Contains experimental features
Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

TiDB now supports the GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() advisory lock functions.

ti-chi-bot · 2022-04-13T15:37:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bb7133
mjonss

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

morgo · 2022-04-13T18:03:00Z

/run-unit-test

sre-bot · 2022-04-13T18:42:49Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/08d449ecf38f4bc7e660cbc95c0ca79f1654e6e6

add similar validation for release_lock lockname as get_lock

yahonda · 2022-04-14T06:19:20Z

Let me confirm if my understanding is correct that based on this implementation, the maximum value of advisory lock timeout only depends on innodb_lock_wait_timeout whose max value is 1073741824 sec, about 34 years.
And it should not be limited by the max value of max-txn-ttl timeout 1 hour.

yahonda · 2022-04-14T07:45:45Z

It looks like the get_lock timeout is limited by 1 hour.

Get lock with second argument as 3800 sec.

mysql> select get_lock('foo', 3800);
+-----------------------+
| get_lock('foo', 3800) |
+-----------------------+
|                     1 |
+-----------------------+
1 row in set, 1 warning (0.00 sec)

Attempt to get the same lock, which results this failure

mysql> select get_lock('FOO', -1);
+---------------------+
| get_lock('FOO', -1) |
+---------------------+
|                   0 |
+---------------------+
1 row in set, 1 warning (1 hour 0.01 sec)
mysql> show warnings;
+---------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                                                            |
+---------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Warning | 1292 | Truncated incorrect get_lock value: '%!s(int64=-00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001)' |
+---------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql>

./bin/tidb-server STDOUT

[2022/04/14 16:37:56.632 +09:00] [INFO] [2pc.go:1092] ["ttlManager live up to its lifetime"] [txnStartTS=432516176655155200] [uptime=3600207] [maxTxnTTL=3600000]
[2022/04/14 16:38:01.229 +09:00] [WARN] [session.go:1946] ["run statement failed"] [schemaVersion=34] [error="[tikv:1205]Lock wait timeout exceeded; try restarting transaction"] [session="{\n  \"currDBName\": \"\",\n  \"id\": 0,\n  \"status\": 3,\n  \"strictMode\": true,\n  \"txn\": \"432516177913446400\",\n  \"user\": null\n}"]

morgo · 2022-04-14T14:56:42Z

Let me confirm if my understanding is correct that based on this implementation, the maximum value of advisory lock timeout only depends on innodb_lock_wait_timeout whose max value is 1073741824 sec, about 34 years. And it should not be limited by the max value of max-txn-ttl timeout 1 hour.

The maximum for innodb_lock_wait_timeout was changed to 1hr to match the default of max-txn-ttl. See: #33473

For this:

mysql> show warnings;
+---------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                                                            |
+---------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Warning | 1292 | Truncated incorrect get_lock value: '%!s(int64=-00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001)' |
+---------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

I'll fix the message. But the warning is intentional - it's not a MySQL behavior, but since we can't do unlimited I thought this was important.

mjonss

When a TiDB server with advisory locks (or any pessimistic transaction) dies (kill -9 for example) how are those locks handled? I did not find any documentation on this...

Mostly questions and still looks good to me, so approved, feel free to consider my suggestions.

expression/builtin_miscellaneous.go

expression/function_traits.go

expression/integration_test.go

mjonss · 2022-04-14T16:30:58Z

expression/integration_test.go

+	tk.MustQuery("SELECT get_lock('mygloballock', 0)").Check(testkit.Rows("0"))  // someone else has the lock
+	tk.MustQuery("SELECT release_lock('mygloballock')").Check(testkit.Rows("0")) // never had the lock


Could not this be used for implementing IS_FREE_LOCK()?
I.e. first check if the session already have the lock, if not do a get_lock with 0 timeout and if we got the lock immediately release it?

Yes, I guess that's possible.

But it's not the most elegant. If I learn the transaction code, I assume I could implement this correctly by checking in TiKV if the lock is held. So I will try and keep out of scope for now.

I thought about this a little more. I think you are right: some implementation is better than no implementation. We know IS_FREE_LOCK() is used in the rails test suite (but not in rails itself).

I will plan to add it for this sprint, but in a separate PR as this one is already getting large.

expression/integration_test.go

session/session.go

morgo · 2022-04-14T17:34:28Z

When a TiDB server with advisory locks (or any pessimistic transaction) dies (kill -9 for example) how are those locks handled? I did not find any documentation on this...

For clean shutdown and kill, I've verified it works. For unsafe shutdown it could take up to the 1hr timeout to clear, I'm not sure if there are mechanisms in place which could shorten it.

morgo · 2022-04-14T17:57:44Z

@mjonss thanks for the review. PTAL again :-)

yahonda · 2022-04-15T04:45:14Z

The maximum for innodb_lock_wait_timeout was changed to 1hr to match the default of max-txn-ttl. See: #33473

Understood the limitation and show warning is helpful when users specified lock timeout value longer than 1 hour.

bb7133

LGTM

bb7133 · 2022-04-29T11:27:06Z

/merge

ti-chi-bot · 2022-04-29T11:27:09Z

This pull request has been accepted and is ready to merge.

Commit hash: 3f7646accea4fc737634eea39c47094a1b982206

morgo · 2022-04-29T14:42:52Z

/merge

ti-chi-bot · 2022-04-29T14:42:55Z

This pull request has been accepted and is ready to merge.

Commit hash: 1911dcf

sre-bot · 2022-04-29T15:42:32Z

TiDB MergeCI notify

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci/integration-cdc-test	🟢 all 34 tests passed	24 min	Existing passed
idc-jenkins-ci-tidb/integration-common-test	🟢 all 11 tests passed	11 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 38 sec	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 12 tests passed	5 min 54 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 33 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 11 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 32 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 32 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 12 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

…ingcap#33947) close pingcap#14994

*: Add support for GET_LOCK() / RELEASE_LOCK()

bfa24c4

morgo requested a review from a team as a code owner April 13, 2022 15:37

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 13, 2022

morgo requested a review from mjonss April 13, 2022 18:00

Fix broken tests

7a57e13

morgo changed the title *: Add support for GET_LOCK() / RELEASE_LOCK() *: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() Apr 14, 2022

fix a bug in release_all_locks() return value, improve tests

c627b99

add similar validation for release_lock lockname as get_lock

morgo force-pushed the add-get-lock branch from 9a00fa8 to c627b99 Compare April 14, 2022 00:40

bb7133 self-requested a review April 14, 2022 03:15

Fix incorrectly formatted warning message, add test

5b8a74b

mjonss approved these changes Apr 14, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 14, 2022

Address PR feedback

544f581

Merge remote-tracking branch 'upstream/master' into add-get-lock

d6ca033

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2022

Merge remote-tracking branch 'upstream/master' into add-get-lock

3851b5b

morgo force-pushed the add-get-lock branch from c7fe36a to 3851b5b Compare April 28, 2022 02:26

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2022

bb7133 approved these changes Apr 29, 2022

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Apr 29, 2022

ti-chi-bot added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 29, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 29, 2022

morgo added 2 commits April 29, 2022 08:37

Merge remote-tracking branch 'upstream/master' into add-get-lock

a54e124

fix merge of pingcap#33569

1911dcf

morgo force-pushed the add-get-lock branch from bd94b82 to 1911dcf Compare April 29, 2022 14:41

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 29, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 29, 2022

Merge branch 'master' into add-get-lock

08d449e

ti-chi-bot merged commit 6d8669e into pingcap:master Apr 29, 2022

morgo mentioned this pull request Apr 29, 2022

add docs for GET_LOCK pingcap/docs#8440

Merged

16 tasks

morgo deleted the add-get-lock branch April 29, 2022 15:54

espresso98 pushed a commit to espresso98/tidb that referenced this pull request Apr 29, 2022

*: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() (p…

00779d6

…ingcap#33947) close pingcap#14994

yahonda mentioned this pull request Jul 21, 2023

Support golang-migrate/migrate that sets SERIALIZABLE isolation level #45487

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() #33947

*: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() #33947

morgo commented Apr 13, 2022 •

edited

Loading

ti-chi-bot commented Apr 13, 2022 •

edited

Loading

morgo commented Apr 13, 2022

sre-bot commented Apr 13, 2022 •

edited

Loading

yahonda commented Apr 14, 2022

yahonda commented Apr 14, 2022

morgo commented Apr 14, 2022

mjonss left a comment

mjonss Apr 14, 2022

morgo Apr 14, 2022

morgo Apr 14, 2022

morgo commented Apr 14, 2022

morgo commented Apr 14, 2022

yahonda commented Apr 15, 2022 •

edited

Loading

bb7133 left a comment

bb7133 commented Apr 29, 2022

ti-chi-bot commented Apr 29, 2022

morgo commented Apr 29, 2022

ti-chi-bot commented Apr 29, 2022

sre-bot commented Apr 29, 2022

		tk.MustQuery("SELECT get_lock('mygloballock', 0)").Check(testkit.Rows("0")) // someone else has the lock
		tk.MustQuery("SELECT release_lock('mygloballock')").Check(testkit.Rows("0")) // never had the lock

*: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() #33947

*: Add support for GET_LOCK() / RELEASE_LOCK() / RELEASE_ALL_LOCKS() #33947

Conversation

morgo commented Apr 13, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 13, 2022 • edited Loading

morgo commented Apr 13, 2022

sre-bot commented Apr 13, 2022 • edited Loading

yahonda commented Apr 14, 2022

yahonda commented Apr 14, 2022

morgo commented Apr 14, 2022

mjonss left a comment

Choose a reason for hiding this comment

mjonss Apr 14, 2022

Choose a reason for hiding this comment

morgo Apr 14, 2022

Choose a reason for hiding this comment

morgo Apr 14, 2022

Choose a reason for hiding this comment

morgo commented Apr 14, 2022

morgo commented Apr 14, 2022

yahonda commented Apr 15, 2022 • edited Loading

bb7133 left a comment

Choose a reason for hiding this comment

bb7133 commented Apr 29, 2022

ti-chi-bot commented Apr 29, 2022

morgo commented Apr 29, 2022

ti-chi-bot commented Apr 29, 2022

sre-bot commented Apr 29, 2022

TiDB MergeCI notify

morgo commented Apr 13, 2022 •

edited

Loading

ti-chi-bot commented Apr 13, 2022 •

edited

Loading

sre-bot commented Apr 13, 2022 •

edited

Loading

yahonda commented Apr 15, 2022 •

edited

Loading