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

Implementation to enforce convenient write locks on Postgres and Sqlite #8004

Conversation

apurva-modi
Copy link
Contributor

@apurva-modi apurva-modi commented Apr 13, 2021

Summary

Created a simple context manager to implement database write lock using atomic transactions.

  • For Sqlite created dummy operation to execute the task.
  • For Postgres implemented advisory lock.

References

Fix for #7891

Reviewer guidance

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

We absolutely need the atomic transaction wrapping to make this work, so that's a must have.

In terms of testing, testing this is going to be a little bit tricky, so I think we should cohack on this to get this out the door!

kolibri/core/device/models.py Outdated Show resolved Hide resolved
kolibri/core/tasks/utils.py Outdated Show resolved Hide resolved
kolibri/core/utils/lock.py Outdated Show resolved Hide resolved
kolibri/core/utils/lock.py Outdated Show resolved Hide resolved
kolibri/core/utils/lock.py Outdated Show resolved Hide resolved
kolibri/core/utils/lock.py Outdated Show resolved Hide resolved
kolibri/core/utils/lock.py Outdated Show resolved Hide resolved
@rtibbles rtibbles added this to the 0.14.7 milestone Apr 14, 2021
…ng atomic transactions, for Sqlite created dummy operation to execute the task and for Postgres implemented advisory lock.
@apurva-modi apurva-modi force-pushed the 7891-db-lock-implementation branch from dbaa452 to b4244f1 Compare April 14, 2021 23:15
@apurva-modi apurva-modi marked this pull request as ready for review April 14, 2021 23:25
@rtibbles
Copy link
Member

The errors are in affected code paths, so we may have something to unpick here. The strangest thing is that the test we created and tested locally was failing too.

I'm rerunning the tests just to be sure.

@apurva-modi
Copy link
Contributor Author

I run tests before pushing.

@rtibbles
Copy link
Member

Yeah, something seems to be behaving differently in CI - I'll check it out in the morning!

with transaction.atomic():
operation = PostgresLock(key=lock_id)
operation.execute()
yield
Copy link
Member

Choose a reason for hiding this comment

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

I don't see operation.revert() here? That seems necessary because the type of advisory lock obtained is a session level lock, which should remain locked for the duration of the connection. I would suggest changing pg_advisory_lock to pg_advisory_xact_lock (reference). Note that for the transaction level lock functions (xact), there aren't corresponding unlock functions because it would be cleared by COMMIT or ROLLBACK of the transaction, including if the connection is closed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks - that's my bad for misleading @apurva-modi here. I'll submit an update.

@rtibbles rtibbles mentioned this pull request Apr 15, 2021
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants