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 serializable isolation db connection for postgres #6591

Merged
merged 4 commits into from
Apr 2, 2020
Merged

Add serializable isolation db connection for postgres #6591

merged 4 commits into from
Apr 2, 2020

Conversation

ralphiee22
Copy link
Contributor

@ralphiee22 ralphiee22 commented Feb 21, 2020

Summary

SQLite and Postgres have different default read isolation levels. SQLite defaults to the "safest" option, which is "serializable", whereas postgres defaults to "read committed". This means that if a transaction reads some data, then in parallel another transaction that writes some data completes, and then the first transaction reads more data, there may be inconsistencies in the two sets of data the first transaction reads -- i.e. it's not a reliable "snapshot" of the database at a particular point in time (c.f. "non-repeatable reads" and "phantom reads").

At the time we built Morango, Kolibri only supported SQlite, which is what we used for our more intensive manual concurrency testing. When we added support for Postgres, we had assumed that it would be at least as robust on this front as SQLite. This type of edge-case mass concurrency testing is very difficult to capture in automated tests, so we don't currently have coverage.

On KDP, this led to some issues where data wasn't being deserialized from the underlying datastore into the Kolibri app models. No data was lost, but it wasn't showing up in the app layer. This was because the deserialization process loops over all the models and then at the very end clears their "dirty" bits. If new records are written into the datastore from an incoming sync in parallel, they may accidentally also have their "dirty bit" cleared, and hence not be deserialized in future batches.

This PR switches the Postgres connection that Morango uses for its large snapshot transactions to have read isolation level of "SERIALIZABLE", which matches SQLite.

Reviewer guidance

References


Contributor Checklist

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

Testing:

  • 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

Reviewer Checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@codecov
Copy link

codecov bot commented Feb 21, 2020

Codecov Report

Merging #6591 into release-v0.13.x will increase coverage by <.01%.
The diff coverage is 83.33%.

Impacted Files Coverage Δ
kolibri/deployment/default/settings/base.py 84.14% <83.33%> (-0.07%) ⬇️
...libri/deployment/default/settings/postgres_test.py 90.9% <83.33%> (-9.1%) ⬇️
kolibri/utils/cli.py 62.74% <0%> (-1.41%) ⬇️
.../core/deviceadmin/management/commands/dbrestore.py 76.56% <0%> (ø) ⬆️
kolibri/core/content/utils/check_schema_db.py 100% <0%> (+8.69%) ⬆️

@indirectlylit
Copy link
Contributor

[DO NOT MERGE YET]

FYI there's a new feature to create un-mergeable draft pull requests.

Unfortunately, it is not possible to turn an existing PR into a draft

Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

image

@indirectlylit indirectlylit merged commit 20ae09a into learningequality:release-v0.13.x Apr 2, 2020
@jamalex jamalex changed the title [DO NOT MERGE YET] add serializable isolation db connection for postgres Add serializable isolation db connection for postgres Jun 30, 2020
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.

3 participants