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

feat: add setOnLocked for SKIP LOCKED and NOWAIT #9317

Merged
merged 1 commit into from Aug 25, 2022

Conversation

taylorhakes
Copy link
Contributor

@taylorhakes taylorhakes commented Aug 24, 2022

Description of change

Fixes #8911

This adds a function addOnLocked to the select query builder. Instead of adding multiple new values to the lockMode enum to support FOR SHARE, FOR KEY SHARE and FOR NO KEY UPDATE, this changes the pattern to add a onLocked to the expression map. Valid values for onLocked are nowait or skip_locked.

As part of this change we should deprecate "pessimistic_partial_write" and pessimistic_write_or_fail because they are no longer necessary with this change and remove them on the next major version change.

Added tests and updated the docs with the new option setOnLocked

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run format to apply prettier formatting
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change (See above)
  • Documentation has been updated to reflect this change (Waiting for approval of this change)
  • The new commits follow conventions explained in CONTRIBUTING.md

@taylorhakes taylorhakes force-pushed the add-on-locked branch 2 times, most recently from 54891b0 to 1eca0c4 Compare August 24, 2022 04:44
@pleerock
Copy link
Member

Someone left "only" in the tests. Merge the latest master into your branch and you will have all tests working.

@pleerock
Copy link
Member

Looks good to me, we also need to update the documentation to document all the new options.

@taylorhakes taylorhakes changed the title feat: add setOnLocked for SKIP LOCKED and NO WAIT feat: add setOnLocked for SKIP LOCKED and NOWAIT Aug 24, 2022
@taylorhakes
Copy link
Contributor Author

Thanks @pleerock. I will add the tests and the docs. Do you specify which version of MySql TypeORM supports? Version 8 of MySQL has FOR SHARE instead of IN SHARE MODE, but not in version 5. Should I assume version 5? Or is there a good way to get the version?

@pleerock
Copy link
Member

If you are talking about documentation, you can simply treat like everyone is using version 8, but if you want to provide additional information regarding to version 5, just put small remarks / comments.

@taylorhakes
Copy link
Contributor Author

taylorhakes commented Aug 24, 2022

Is there a way to get the version of the DB in code? The code for mysql currently uses IN SHARE MODE, but that statement doesn't support SKIP LOCKED or NOWAIT. I would like to use the statement FOR SHARE instead for mysql, but it requires version 8 of MYSQL. Is there a way to get the MySQL version, so I can change the statement based on the version? If I just change it to FOR SHARE, all users on MySQL 5 can no longer use the "pessimistic_read" option because it adds a SQL statement that is incompatible with their version of MySQL.

If this change needs to be backwards compatible, I cannot support MySQL on "pessimistic_read" because we would have to use IN SHARE MODE to support MySQL 5

@pleerock
Copy link
Member

We have a code that does version check in MysqlDriver.connect, but the version itself doesn't get stored anywhere. You can store it in the driver itself for the future uses.

@taylorhakes
Copy link
Contributor Author

@pleerock Should be good to review now. I added some comments about deprecating "pessimistic_partial_write" and "pessimistic_write_or_fail". You can remove them in the next major version change.

@taylorhakes taylorhakes force-pushed the add-on-locked branch 4 times, most recently from 5ab4108 to d1ec672 Compare August 25, 2022 05:08
@pleerock
Copy link
Member

Can you please merge the latest master into your branch? Tests are failing because of it...

@taylorhakes
Copy link
Contributor Author

@pleerock Updated to latest master

@taylorhakes
Copy link
Contributor Author

This is my first time working on this codebase. I think it probably makes sense to refactor some of the query builders to have a different file per driver (not this PR obviously). The if statements inside the SelectBuilder.js are getting a little unwieldy.

@pleerock
Copy link
Member

This is my first time working on this codebase. I think it probably makes sense to refactor some of the query builders to have a different file per driver (not this PR obviously). The if statements inside the SelectBuilder.js are getting a little unwieldy.

I know. This codebase is like 8 years old and most of its parts need refactoring. Unfortunately I don't have much time to do it (plus you always afraid to break something, too many people depend on it). And unfortunately when last time I let other people to completely refactor some of the code, now I cannot understand the details of their re-write in order to merge changes / fix issues related to their code. And these people are not available anymore (or don't respond). So better to keep things this way for now. Unfortunate, but for the good.

@pleerock
Copy link
Member

I'll merge it. Thanks for the contribution!

@pleerock pleerock merged commit 68e8f22 into typeorm:master Aug 25, 2022
@taylorhakes
Copy link
Contributor Author

@pleerock Really appreciate the quick review and merge! I understand open source is a ton of work (I have a few libraries of my own).

How often do you do releases?

@pleerock
Copy link
Member

Usually once in month-two. Next release will be this week.

nordinh pushed a commit to nordinh/typeorm that referenced this pull request Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SKIP LOCKED with FOR SHARE in Postgres
2 participants