fix: support MySQL user-level lock functions#24909
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
I found multiple blocking issues in the current implementation.
-
The named-lock state is only a package-global in-memory map inside one CN process. In MatrixOne this means two sessions routed to different CNs can both acquire the same lock name at the same time, so the core exclusivity guarantee is broken for real cross-session coordination.
-
Locks are never released when the owning session disconnects. The only mutations of the new lock map are explicit acquire/release calls, and this PR does not add any session-teardown cleanup path. So a client that exits without
RELEASE_LOCK()can leave the lock stuck forever until the CN restarts, which is not compatible with MySQL named-lock semantics. -
GET_LOCK(name, timeout)handles timeout semantics incorrectly:timeout <= 0returns0immediately, but MySQL compatibility requirestimeout = 0to mean no-wait and negative timeout to mean wait indefinitely (subject to session cancellation). -
The unhappy-path coverage is still thin around the new blocking behavior. There are no tests for disconnect cleanup, successful wait-then-acquire, timeout expiry, context cancellation while waiting, or the reentrant refcount path.
Because the first two are real behavior bugs and the third is a compatibility bug in the exposed SQL contract, I am requesting changes.
jiangxinmeng1
force-pushed
the
fix-user-level-lock-functions
branch
from
c6ab3b7 to
5ca0b17
Compare
|
Updated the PR to address the review request:
Tests run:
|
jiangxinmeng1
requested review from
LeftHandCold,
aptend,
gouhongshen and
iamlinjunhong
as code owners
|
|
…ock-functions # Conflicts: # pkg/sql/plan/function/function_id.go # pkg/sql/plan/function/function_id_test.go
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the latest fix approach, ignoring CI status. The implementation now addresses the earlier correctness issues around distributed locking, session cleanup, timeout semantics, RELEASE_LOCK NULL/0 semantics, empty/overlong names, multibyte length, and case-insensitive lock names. However, this PR introduces new public SQL functions (GET_LOCK, RELEASE_LOCK, IS_FREE_LOCK) for Prisma/MySQL compatibility and currently adds only Go unit tests; there is no BVT/end-to-end SQL case under test/distributed/cases. Please add at least one function BVT covering the SQL contract, e.g. GET_LOCK('prisma_migrate_lock', 0), IS_FREE_LOCK before/after acquire/release, RELEASE_LOCK for held and never-created locks, and NULL argument behavior. The fake-lockservice unit tests are strong, but they do not verify SQL parsing/planning/execution or result NULL/0/1 behavior through mo-tester.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head with CI status ignored. The previous issues are addressed: the implementation uses the lockservice for cross-CN coordination, releases session-owned locks on Session.Close, handles timeout semantics, returns the correct RELEASE_LOCK 1/0/NULL states, normalizes names case-insensitively, validates empty/overlong/multibyte names, and preserves reentrant ownership. The new BVT now covers the public SQL path for GET_LOCK/RELEASE_LOCK/IS_FREE_LOCK, including NULL and never-created lock behavior. No blocking issue found in the fix approach.
There was a problem hiding this comment.
Pull request overview
This PR adds MySQL-compatible user-level advisory lock built-ins (GET_LOCK, RELEASE_LOCK, IS_FREE_LOCK) so clients like Prisma Migrate can acquire a named lock before running migrations, backed by the existing lockservice and released on session close.
Changes:
- Register new built-in functions and function IDs for
get_lock,release_lock, andis_free_lock. - Implement connection-owned named lock semantics (acquire, release, free check, ref-counting, and session-close cleanup).
- Add both BVT distributed SQL cases and Go unit tests covering core behaviors (acquire/release/free, contention, wait/timeout/cancel, NULL handling, length/case rules).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/distributed/cases/function/func_user_lock.test | New distributed BVT case for user-level lock functions behavior. |
| test/distributed/cases/function/func_user_lock.result | Expected outputs for the new BVT case. |
| pkg/sql/plan/function/list_builtIn.go | Registers GET_LOCK / RELEASE_LOCK / IS_FREE_LOCK built-ins and their overloads. |
| pkg/sql/plan/function/function_test.go | Adds a unit test to ensure the built-ins are registered as expected. |
| pkg/sql/plan/function/function_id.go | Adds function IDs and name→ID registrations for the new functions. |
| pkg/sql/plan/function/function_id_test.go | Updates function ID expectations to include the new IDs. |
| pkg/sql/plan/function/func_unary.go | Implements lock acquisition/release/free-check and per-session cleanup logic. |
| pkg/sql/plan/function/func_unary_test.go | Adds unit tests for lock behavior (contention, wait, timeout/cancel, refcount, name rules). |
| pkg/frontend/session.go | Ensures user-level locks held by a session are released during session close. |
…ock-functions # Conflicts: # pkg/sql/plan/function/function_id_test.go
|
@Mergifyio queue |
Merge Queue Status
This pull request spent 22 seconds in the queue, including 4 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) does this PR fix or relate to?
Fixes #24728
What this PR does / why we need it:
What
Why
Fixes #24728. Prisma Migrate calls GET_LOCK before applying migrations.
Tests