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

session,expression: Support IS_USED_LOCK function #44495

Merged
merged 6 commits into from
Jun 21, 2023

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Jun 7, 2023

What problem does this PR solve?

Issue Number: close #44493

Problem Summary:

What is changed and how it works?

TiDB, Session 1

sql> select is_used_lock('foo');
+---------------------+
| is_used_lock('foo') |
+---------------------+
|                NULL |
+---------------------+
1 row in set (0.0037 sec)

sql> select get_lock('foo', 10);
+---------------------+
| get_lock('foo', 10) |
+---------------------+
|                   1 |
+---------------------+
1 row in set (0.0032 sec)

sql> select is_used_lock('foo');
+---------------------+
| is_used_lock('foo') |
+---------------------+
|       2199023255557 |
+---------------------+
1 row in set (0.0005 sec)

sql> select connection_id();
+-----------------+
| connection_id() |
+-----------------+
|   2199023255557 |
+-----------------+
1 row in set (0.0004 sec)

TiDB, Session 2

sql> select is_used_lock('foo');
+---------------------+
| is_used_lock('foo') |
+---------------------+
|                   1 |
+---------------------+
1 row in set (1.0120 sec)

sql> select is_used_lock('bar');
+---------------------+
| is_used_lock('bar') |
+---------------------+
|                NULL |
+---------------------+
1 row in set (0.0033 sec)

MySQL

sql> select is_used_lock('foo');
+---------------------+
| is_used_lock('foo') |
+---------------------+
|                NULL |
+---------------------+
1 row in set (0.0007 sec)

sql> select get_lock('foo',10);
+--------------------+
| get_lock('foo',10) |
+--------------------+
|                  1 |
+--------------------+
1 row in set (0.0007 sec)

sql> select is_used_lock('foo');
+---------------------+
| is_used_lock('foo') |
+---------------------+
|                  36 |
+---------------------+
1 row in set (0.0007 sec)

sql> select connection_id();
+-----------------+
| connection_id() |
+-----------------+
|              36 |
+-----------------+
1 row in set (0.0006 sec)

sql> select version();
+-----------+
| version() |
+-----------+
| 8.0.33    |
+-----------+
1 row in set (0.0007 sec)

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • 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
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

Support for the `IS_USED_LOCK()` and `IS_FREE_LOCK()` functions was added

@dveeden dveeden requested a review from mjonss June 7, 2023 17:05
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 7, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2023
@dveeden
Copy link
Contributor Author

dveeden commented Jun 7, 2023

The problem with the current implementation is that lock.owner is only available to the current session. For other sessions (possibly on other nodes) this basically tries to get and release a transaction to see if a lock exists. The problem with this is that it doesn't return information on who owns the lock. Now it always returns 1 instead of the proper process ID because of that.

  • Could we check lock metadata instead of lock/release to check if a lock exists?
  • We could add another column to the mysql.advisory_locks table to record the owner. However as the transaction never gets commited it would be difficult to get the record from other sessions/nodes.

@dveeden
Copy link
Contributor Author

dveeden commented Jun 8, 2023

/test all

@dveeden dveeden requested a review from bb7133 June 8, 2023 11:39
@dveeden
Copy link
Contributor Author

dveeden commented Jun 8, 2023

The evalInt() now does a int64() on the owner pid before returning it. So sessions with an ID above MaxInt64 would likely have issues. Not sure how to properly return higher values.

Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add IS_FREE_LOCK() as an inverse of IS_USED_LOCK()?

session/advisory_locks.go Show resolved Hide resolved
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 8, 2023
@dveeden
Copy link
Contributor Author

dveeden commented Jun 8, 2023

Maybe also add IS_FREE_LOCK() as an inverse of IS_USED_LOCK()?

Done in edb9729

MySQL

sql> select is_free_lock('test');
+----------------------+
| is_free_lock('test') |
+----------------------+
|                    1 |
+----------------------+
1 row in set (0.0004 sec)

sql> select get_lock('test',1);
+--------------------+
| get_lock('test',1) |
+--------------------+
|                  1 |
+--------------------+
1 row in set (0.0007 sec)

sql> select is_free_lock('test');
+----------------------+
| is_free_lock('test') |
+----------------------+
|                    0 |
+----------------------+
1 row in set (0.0064 sec)

sql> select version();
+-----------+
| version() |
+-----------+
| 8.0.33    |
+-----------+
1 row in set (0.0007 sec)

TiDB

+----------------------+
| is_free_lock('test') |
+----------------------+
|                    1 |
+----------------------+
1 row in set (0.0042 sec)

sql> select get_lock('test',1);
+--------------------+
| get_lock('test',1) |
+--------------------+
|                  1 |
+--------------------+
1 row in set (0.0026 sec)

sql> select is_free_lock('test');
+----------------------+
| is_free_lock('test') |
+----------------------+
|                    0 |
+----------------------+
1 row in set (0.0010 sec)

@dveeden
Copy link
Contributor Author

dveeden commented Jun 8, 2023

/test all

@dveeden dveeden requested a review from tiancaiamao June 8, 2023 16:00
@dveeden dveeden marked this pull request as ready for review June 8, 2023 16:01
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2023
@dveeden
Copy link
Contributor Author

dveeden commented Jun 8, 2023

/retest

@ti-chi-bot ti-chi-bot bot added the lgtm label Jun 21, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, tiancaiamao

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 21, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 21, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-06-08 14:02:10.099037304 +0000 UTC m=+6944.346959128: ☑️ agreed by mjonss.
  • 2023-06-21 02:04:12.180955518 +0000 UTC m=+154817.582205967: ☑️ agreed by tiancaiamao.

@tiancaiamao
Copy link
Contributor

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support IS_USED_LOCK() and IS_FREE_LOCK() functions
3 participants