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

Support for NOWAIT / SKIP LOCKED options in locking clause #2037

Open
clarkdave opened this issue Apr 26, 2018 · 12 comments
Open

Support for NOWAIT / SKIP LOCKED options in locking clause #2037

clarkdave opened this issue Apr 26, 2018 · 12 comments

Comments

@clarkdave
Copy link

Issue type:

[x] feature request

Database system/driver:

[x] mssql
[x] mysql / mariadb
[x] oracle
[x] postgres

TypeORM version:

[x] latest


PostgreSQL, MySQL, MSSQL and Oracle (I think), and perhaps others, support NOWAIT and SKIP LOCKED options for row locking. The PostgreSQL documentation has a good explanation of these options, but in summary:

nowait

With the NOWAIT option, the statement reports an error if it is unable to take the lock.

SELECT * FROM jobs WHERE id = 5 FOR UPDATE NOWAIT

skip locked

With the SKIP LOCKED option, any selected rows that cannot be immediately locked are skipped.

SELECT * FROM jobs WHERE id = 5 FOR UPDATE SKIP LOCKED

Currently, TypeORM only supports the bare FOR UPDATE clause, via the pessimistic_write lock mode.

Some additional considerations:

  • These options are, in some databases, relatively new. SKIP LOCKED appeared in PostgreSQL 9.5 (2016). I think support in MySQL is even more recent. I'm not sure what TypeORM's philosophy is on supporting newer SQL features?
  • NOWAIT, by design, returns an error if the row is locked, so it seems necessary that an implementation of this option would have to handle this gracefully (a new Error type?)
@pleerock
Copy link
Member

Feel free to contribute. Probably we can add forUpdate method into SelectQueryBuilder, e.g.

createQueryBuilder("job")
   .where("job.id = :id", { id })
   .forUpdate("NOWAIT")
   .getMany();

@clarkdave
Copy link
Author

clarkdave commented Apr 27, 2018

@pleerock These lock options aren't specific to FOR UPDATE - they can be used with any "lock strength", and also combined with an OF table_name to limit the lock to a specific table when multiple are involved.

For example, all of the following queries are valid:

SELECT * FROM table WHERE id = 1 FOR UPDATE;
SELECT * FROM table WHERE id = 1 FOR SHARE SKIP LOCKED;
SELECT * FROM table WHERE id = 1 FOR UPDATE NOWAIT;
SELECT * FROM table WHERE id = 1 FOR KEY SHARE SKIP LOCKED;

SELECT a.*, b.*
  FROM a
  LEFT JOIN b ON b.id = a.b_id
  FOR UPDATE OF a
  SKIP LOCKED;

With that in mind, could TypeORM support a more generic way to set the locking clause for a query? As an example from the wild, ActiveRecord lets you pass through an arbitrary string:

User
  .lock('FOR UPDATE OF users SKIP LOCKED')
  .where(:id => 1)
  .first

Maybe this could be supported in TypeORM by adding another override to setLock which accepts a raw string to use as the locking clause, as with other methods like addGroupBy:

const user = await connection
  .getRepository(User)
  .createQueryBuilder('user')
  .leftJoinAndSelect('user.photos', 'photo')
  .where({ id: 1 })
  .setLock('FOR UPDATE OF user SKIP LOCKED')
  // .setLock('FOR KEY SHARE OF user')
  // .setLock('FOR NO KEY UPDATE NOWAIT')
  // .setLock('FOR SHARE OF user')
  .getOne()

Thoughts?

@clarkdave
Copy link
Author

BTW I'm happy to take this on and submit a PR once we agree the implementation 👍

@pleerock
Copy link
Member

To not confuse things I would go this way:

  1. deprecate setLock
  2. create lockMode() method that will do same as setLock right now, e.g. rename setLock method
  3. create lock() method that accepts string and do what you proposed

What do you think?

@azigelman
Copy link

+1

Any updates?

@pleerock
Copy link
Member

This issue is opened for contributions.

clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 28, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 28, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 28, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 28, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 29, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 29, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 29, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 29, 2018
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Jan 4, 2019
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Jan 4, 2019
@NickCarton
Copy link

I love this idea, excited to see it added in the future!

clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 19, 2019
clarkdave added a commit to loyaltylion/typeorm that referenced this issue Nov 19, 2019
@Nopik
Copy link
Contributor

Nopik commented Apr 14, 2020

+1 any updates?

This also seem to not support directly the 'select * ... for update skip locked' on psql - unless the lockClause is the supposed way to do this?

@Nopik
Copy link
Contributor

Nopik commented Apr 21, 2020

@clarkdave do you consider this feature finished? Can you open PR for this? Or there is something still missing?

@Nopik
Copy link
Contributor

Nopik commented Apr 21, 2020

I've added #5927 as a smaller PR.

pamdesilva pushed a commit to loyaltylion/typeorm that referenced this issue Nov 18, 2020
pamdesilva pushed a commit to loyaltylion/typeorm that referenced this issue Nov 18, 2020
TomVance pushed a commit to loyaltylion/typeorm that referenced this issue Nov 20, 2020
TomVance pushed a commit to loyaltylion/typeorm that referenced this issue Nov 20, 2020
@thehappycoder
Copy link

That's awesome but I realised I also need to specify rows of what tables need to be locked by using FOR UPDATE OF tablename https://stackoverflow.com/questions/6998372/sql-semantics-for-for-update-join

@taylorhakes
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment