Reduce Postgres transaction isolation level#1860
Merged
Conversation
t-bast
previously approved these changes
Jul 5, 2021
Member
t-bast
left a comment
There was a problem hiding this comment.
LGTM, I don't see why we'd need a stronger isolation either.
Let's run with that for a while and see how it behaves.
f6a45d9 to
8fb04dc
Compare
Member
Author
|
Rebased on #1862. |
8fb04dc to
aa9a5ee
Compare
I was able to reproduce #1856 by replaying the "concurrent channel updates" test with hardcoded additional delays in the database code. It was due to a conflict between `addOrUpdateChannel` and `updateChannelMetaTimestampColumn`. The two calls run in parallel and the latter completed before the former, causing it to fail. Reducing the isolation level makes the problem disappear. We reduce the transaction isolation level from `SERIALIZABLE` to `READ_COMMITTED`. Note that [*]: > Read Committed is the default isolation level in PostgreSQL. I'm not sure why we were using a stricter isolation level than the default one, since we only use very basic queries. Doc does say that: > This behavior makes Read Committed mode unsuitable for commands that involve complex search conditions; however, it is just right for simpler cases To make sure this didn't cause regression withe the locking mechanism, I wrote an additional test specifically on the `withLock` method. Here is what the doc says on the `INSERT ON CONFLICT DO UPDATE` statement, which we use for `addOrUpdateChannel`: > INSERT with an ON CONFLICT DO UPDATE clause behaves similarly. In Read Committed mode, each row proposed for insertion will either insert or update. Unless there are unrelated errors, one of those two outcomes is guaranteed. If a conflict originates in another transaction whose effects are not yet visible to the INSERT, the UPDATE clause will affect that row, even though possibly no version of that row is conventionally visible to the command. In the scenario described above, the `addOrUpdate` will update the row which timestamp was updated in parallel by the `updateChannelMetaTimestampColumn`, and it's exactly what we want. Fixes #1856. [*] https://www.postgresql.org/docs/13/transaction-iso.html
aa9a5ee to
a0535dd
Compare
Member
Author
|
Rebased on master. |
t-bast
approved these changes
Jul 8, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I was able to reproduce #1856 by replaying the "concurrent channel
updates" test with hardcoded additional delays in the database code. It
was due to a conflict between
addOrUpdateChannelandupdateChannelMetaTimestampColumn. The two calls run in parallel andthe latter completed before the former, causing it to fail. Reducing
the isolation level makes the problem disappear.
We reduce the transaction isolation level from
SERIALIZABLEtoREAD_COMMITTED. Note that [*]:I'm not sure why we were using a stricter isolation level than the
default one, since we only use very basic queries. Doc does say that:
To make sure this didn't cause regression withe the locking
mechanism, I wrote an additional test specifically on the
withLockmethod.
Here is what the doc says on the
INSERT ON CONFLICT DO UPDATEstatement, which we use for
addOrUpdateChannel:In the scenario described above, the
addOrUpdatewill update the rowwhich timestamp was updated in parallel by the
updateChannelMetaTimestampColumn, and it's exactly what we want.Fixes #1856.
[*] https://www.postgresql.org/docs/13/transaction-iso.html