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

Add support for SELECT FOR UPDATE #1065

Closed
wants to merge 5 commits into from

Conversation

dkopitsa
Copy link
Contributor

@dkopitsa dkopitsa commented Aug 8, 2024

Add clause for_update(nowait: bool = False, skip_locked: bool = False, of=()) for objects and select query.

Similar to django's select_for_update
Closes #932

Example usage:

Get one row matching the status, locks it for update, and skips any already locked rows.

async with DB.transaction():
    await Outbox.objects().where(Outbox.status == 'failed').limit(1).for_update(skip_locked=True).first()

Ensure that the selected object is locked and cannot be modified by other transactions

async with DB.transaction():
     await org = Organization.objects().where(Organization.code == code).for_update()
     # do some work with object ...
     await org.save()

tests/table/test_select.py Outdated Show resolved Hide resolved
@sinisaos
Copy link
Member

sinisaos commented Aug 9, 2024

@dkopitsa Can you please run locally ./scripts/format.sh and then ./scripts/lint.sh to fix linter/format issue and then make another commit. After that everything should be ok because tests are passing.

@dkopitsa dkopitsa force-pushed the select_for_update branch from 6296078 to fdd7f53 Compare August 9, 2024 07:33
@dkopitsa
Copy link
Contributor Author

dkopitsa commented Aug 9, 2024

@dkopitsa Can you please run locally ./scripts/format.sh and then ./scripts/lint.sh to fix linter/format issue and then make another commit. After that everything should be ok because tests are passing.

Done. Sorry about the linters; I wanted to fix the tests quickly and used GitHub's web editor.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 86.36364% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.47%. Comparing base (4ed6938) to head (fdd7f53).
Report is 11 commits behind head on master.

Files Patch % Lines
piccolo/query/mixins.py 87.87% 4 Missing ⚠️
piccolo/query/methods/objects.py 50.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1065      +/-   ##
==========================================
- Coverage   92.66%   88.47%   -4.20%     
==========================================
  Files         115      116       +1     
  Lines        8444     8543      +99     
==========================================
- Hits         7825     7558     -267     
- Misses        619      985     +366     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sinisaos
Copy link
Member

sinisaos commented Aug 9, 2024

Done. Sorry about the linters; I wanted to fix the tests quickly and used GitHub's web editor.

No worries. Thanks for your PR. Looks good to me, but you'll have to wait for @dantownsend proper review.

piccolo/query/mixins.py Outdated Show resolved Hide resolved
piccolo/query/mixins.py Outdated Show resolved Hide resolved
piccolo/query/mixins.py Outdated Show resolved Hide resolved
piccolo/query/mixins.py Show resolved Hide resolved
@dkopitsa dkopitsa force-pushed the select_for_update branch from 0d0cb80 to 5e3c4f0 Compare August 9, 2024 09:15
@deliro
Copy link

deliro commented Sep 2, 2024

Any updates here?

@dkopitsa
Copy link
Contributor Author

Hi @dantownsend , could you please review the PR? If there's anything that needs further work or adjustments, let me know, and I'll make the necessary changes

Copy link
Member

@dantownsend dantownsend left a comment

Choose a reason for hiding this comment

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

Hi - sorry for the delay with this. I think the PR was opened while I was busy adding MFA to Piccolo Admin.

It looks really good, but my main concern is whether we should refactor it slightly, so instead of the method being for_update, instead it should be something like this:

await Band.select().locking('for update')

Or even:

await Band.select().for('update')

Just because there are multiple strengths of locks:

https://www.postgresql.org/docs/current/sql-select.html#SQL-FOR-UPDATE-SHARE

We could support them all in one method, rather than having to add more methods in the future for each one.

I quite like the idea of calling the method for but it clashes with the Python for keyword, which can sometimes cause weird bugs. So lock_for might be better.

What do you think?

@@ -0,0 +1,62 @@
.. _limit:
Copy link
Member

Choose a reason for hiding this comment

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

We should get rid of this.

docs/src/piccolo/query_clauses/for_update.rst Outdated Show resolved Hide resolved
docs/src/piccolo/query_clauses/for_update.rst Outdated Show resolved Hide resolved
docs/src/piccolo/query_clauses/for_update.rst Outdated Show resolved Hide resolved
docs/src/piccolo/query_clauses/for_update.rst Outdated Show resolved Hide resolved
piccolo/query/methods/objects.py Outdated Show resolved Hide resolved
docs/src/piccolo/query_clauses/for_update.rst Outdated Show resolved Hide resolved
@dkopitsa
Copy link
Contributor Author

Good point. Personally, I don't use anything other than 'update', but it could be useful, especially for the ORM.

await Band.select().lock_for('update', skip_locked=True)

looks good to me. I'll make the necessary changes.

@dkopitsa
Copy link
Contributor Author

I fixed the formatting in test_select.py, please run workflow again.

@dantownsend
Copy link
Member

@dkopitsa This looks great - thanks a lot!

I'll test it out locally.

@dantownsend
Copy link
Member

@dkopitsa I've merged this in now, via another branch.

I just changed the method name from lock_for to lock_rows. I realised when looking at the docs that this looked a bit off when no lock strength was specified:

await Band.select().lock_for()

So now it's this instead:

await Band.select().lock_rows()

Thanks a lot for all the effort you put into this - it's a very nice feature to have 👍

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.

Pessimistic locking: select for update
5 participants