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

UCP: Support get_lock and release_lock functions #14994

Closed
qw4990 opened this issue Feb 28, 2020 · 18 comments · Fixed by #33947
Closed

UCP: Support get_lock and release_lock functions #14994

qw4990 opened this issue Feb 28, 2020 · 18 comments · Fixed by #33947
Assignees
Labels
feature/discussing This feature request is discussing among product managers type/enhancement The issue or PR belongs to an enhancement. type/feature-request Categorizes issue or PR as related to a new feature.

Comments

@qw4990
Copy link
Contributor

qw4990 commented Feb 28, 2020

Description

Support the get_lock and release_lock function, which can be used to implement application locks or to simulate record locks.

Score

  • 3488

Mentor(s)

Contact the mentors: #tidb-challenge-program channel in TiDB Community Slack Workspace

Recommended Skills

  • SQL
  • Golang

Learning Materials

@qw4990 qw4990 added the type/enhancement The issue or PR belongs to an enhancement. label Feb 28, 2020
@qw4990 qw4990 changed the title UCP: Support the release_lock functions UCP: Support get_lock and release_lock functions Mar 4, 2020
@AndrewDi
Copy link
Contributor

AndrewDi commented Mar 6, 2020

@qw4990 Do we always need to consider all lock related function?

GET_LOCK() | Get a named lock
IS_FREE_LOCK() | Whether the named lock is free
IS_USED_LOCK() | Whether the named lock is in use; return connection identifier if true
RELEASE_ALL_LOCKS() | Release all current named locks
RELEASE_LOCK() | Release the named lock

@AndrewDi
Copy link
Contributor

AndrewDi commented Mar 6, 2020

/pick-up-challenge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

"easy" issue is not available since you have got 200 score in this repo. Please pickup "medium" or "hard" directly. Or you can pick up "easy" Issue in other repos. Thank you.

@you06
Copy link
Contributor

you06 commented Mar 6, 2020

@AndrewDi try again please.

@AndrewDi
Copy link
Contributor

AndrewDi commented Mar 6, 2020

/pick-up-challenge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

@AndrewDi pick up issue success

@AndrewDi
Copy link
Contributor

/give-up-challenge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 10, 2020

@AndrewDi give up issue success

@zz-jason zz-jason added the type/feature-request Categorizes issue or PR as related to a new feature. label Jun 8, 2020
@ghost ghost added the correctness label Aug 3, 2020
@ghost
Copy link

ghost commented Aug 3, 2020

It is important to clarify that this can cause correctness issues for applications. They depend on get_lock to act like a mutex and serialize access to some shared resource. By TiDB always returning that the lock is free, the application will have data races and could generate incorrect behavior.

@zz-jason zz-jason added feature/reviewing This feature request is reviewing by product managers feature/discussing This feature request is discussing among product managers and removed feature/reviewing This feature request is reviewing by product managers labels Aug 6, 2020
@ghost ghost removed the correctness label Aug 11, 2020
@ghost
Copy link

ghost commented Aug 11, 2020

I correct my previous statement. It looks like at some point get_lock started returning errors by default (which is great! It helps make sure users are not caught by accident):

mysql> SELECT get_lock('abcd');
ERROR 1235 (42000): function get_lock has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions
mysql> SELECT tidb_version()\G
*************************** 1. row ***************************
tidb_version(): Release Version: v4.0.0-beta.2-932-g8978773f5
Edition: Community
Git Commit Hash: 8978773f5e3d43a100550e6babea9904a99e5938
Git Branch: master
UTC Build Time: 2020-08-10 12:35:53
GoVersion: go1.13
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false
1 row in set (0.00 sec)

Thus, I have removed the correctness label. I still think this feature is very useful since Ruby on Rails, Flyway use it for migrations. But the current behavior is no longer high-risk.

@realxujiang
Copy link

This will make Flyway unusable.

image

@morgo
Copy link
Contributor

morgo commented Sep 23, 2021

@realxujiang For Flyway, please use 7.11 and above. It uses an alternative implementation of lock: flyway/flyway@77b50ee

@yahonda
Copy link
Member

yahonda commented Nov 2, 2021

get_lock functions is required to support Prisma Migrate. https://www.prisma.io/docs/concepts/components/prisma-migrate

  1. Startup TiDB v5.2.2 using tiup playground
% tiup playground v5.2.2
  1. Follow the Prisma getting started steps as follows.

https://www.prisma.io/docs/getting-started/setup-prisma/start-from-scratch/relational-databases-typescript-mysql

2.1 prisma version

% npx prisma --version
Environment variables loaded from .env
prisma                  : 3.3.0
@prisma/client          : Not found
Current platform        : darwin
Query Engine (Node-API) : libquery-engine 33838b0f78f1fe9052cf9a00e9761c9dc097a63c (at node_modules/@prisma/engines/libquery_engine-darwin.dylib.node)
Migration Engine        : migration-engine-cli 33838b0f78f1fe9052cf9a00e9761c9dc097a63c (at node_modules/@prisma/engines/migration-engine-darwin)
Introspection Engine    : introspection-core 33838b0f78f1fe9052cf9a00e9761c9dc097a63c (at node_modules/@prisma/engines/introspection-engine-darwin)
Format Binary           : prisma-fmt 33838b0f78f1fe9052cf9a00e9761c9dc097a63c (at node_modules/@prisma/engines/prisma-fmt-darwin)
Default Engines Hash    : 33838b0f78f1fe9052cf9a00e9761c9dc097a63c
Studio                  : 0.437.0
%

2.2 npx prisma migrate dev --name init fails

% npx prisma migrate dev --name init
Environment variables loaded from .env
Prisma schema loaded from prisma/schema.prisma
Datasource "db": MySQL database "test" at "localhost:4000"

Error: function get_lock has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions
   0: migration_core::api::ApplyMigrations
             at migration-engine/core/src/api.rs:84

%

@AmreeshTyagi
Copy link

AmreeshTyagi commented Nov 5, 2021

@yahonda Yes. This feature is needed for prisma as well. Waiting tidb to support it.

For now I have solved it by enabling noop functions in tidb.
Make sure to read Warning in this case as per tidb documentation.

SET GLOBAL tidb_enable_noop_functions=1;

After that I was able to execute prisma client, which created all tables but got Duplicate foreign key constraint name . So, I upgraded my prisma to latest version 3.4.0, in which prisma is supporting named mapping. And it worked perfectly.

I haven't checked any queries as it a new database for my new project.

@yahonda
Copy link
Member

yahonda commented Nov 5, 2021

Thanks for the info and I was aware setting tidb_enable_noop_functions=1; can workaround it. I added my comment because I wanted to add other use cases why this feature is necessary.

@yahonda
Copy link
Member

yahonda commented Jan 17, 2022

While I was preparing pingcap/docs-appdev#71 I've found elixir Ecto also needs release_lock support.

% mix ecto.migrate

** (MatchError) no match of right hand side value: {:error, %MyXQL.Error{connection_id: 17, message: "function release_lock has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions", mysql: %{code: 1235, name: nil}, statement: "SELECT RELEASE_LOCK(\"ecto_Friends.Repo\")"}}
    (ecto_sql 3.7.1) lib/ecto/adapters/myxql.ex:240: anonymous fn/4 in Ecto.Adapters.MyXQL.lock_for_migrations/3
    (ecto_sql 3.7.1) lib/ecto/adapters/sql.ex:1021: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
    (db_connection 2.4.1) lib/db_connection.ex:1531: DBConnection.run_transaction/4
    (ecto_sql 3.7.1) lib/ecto/adapters/myxql.ex:237: Ecto.Adapters.MyXQL.lock_for_migrations/3
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:526: Ecto.Migrator.lock_for_migrations/4
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:419: Ecto.Migrator.run/4
    (ecto_sql 3.7.1) lib/ecto/migrator.ex:146: Ecto.Migrator.with_repo/3
    (ecto_sql 3.7.1) lib/mix/tasks/ecto.migrate.ex:138: anonymous fn/5 in Mix.Tasks.Ecto.Migrate.run/2

setting tidb_enable_noop_functions=1; is still a workaround.

@yahonda
Copy link
Member

yahonda commented Jan 17, 2022

Here is the code which runs get_lock and release_lock.

https://github.com/elixir-ecto/ecto_sql/blob/7bef2cf7ea016f2311241c6a213bdb3284b62fda/lib/ecto/adapters/myxql.ex#L237-L246

      transaction(meta, opts, fn ->
        lock_name = "\"ecto_#{inspect(repo)}\""

        try do
          {:ok, _} = Ecto.Adapters.SQL.query(meta, "SELECT GET_LOCK(#{lock_name}, -1)", [], opts)
          fun.()
        after
          {:ok, _} = Ecto.Adapters.SQL.query(meta, "SELECT RELEASE_LOCK(#{lock_name})", [], opts)
        end
      end)

@yahonda
Copy link
Member

yahonda commented Mar 15, 2022

This is the Ruby on Rails code which is making use of GET_LOCK and RELEASE_LOCK.

https://github.com/rails/rails/blob/f01fee7fc49371c28a1a59ed406f7ee00fcda3bf/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb#L150-L156

      def get_advisory_lock(lock_name, timeout = 0) # :nodoc:
        query_value("SELECT GET_LOCK(#{quote(lock_name.to_s)}, #{timeout})") == 1
      end

      def release_advisory_lock(lock_name) # :nodoc:
        query_value("SELECT RELEASE_LOCK(#{quote(lock_name.to_s)})") == 1
      end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/discussing This feature request is discussing among product managers type/enhancement The issue or PR belongs to an enhancement. type/feature-request Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.