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(core): allow specifying transaction isolation level #2116

Merged
merged 3 commits into from
Apr 19, 2023

Conversation

domsj
Copy link
Contributor

@domsj domsj commented Apr 10, 2023

The default isolation level in some databases (see e.g. for postgres https://www.postgresql.org/docs/current/transaction-iso.html - READ COMMITTED is default) allows several anomalies, leading to incorrect data. https://en.wikipedia.org/wiki/Isolation_%28database_systems%29 for more context (or do some googling).

I think it would make sense to at least easily allow users to opt in to stronger isolation levels, and preferably do so by default (picking a lower isolation level should only be done when really needed and when you understand what you're doing).

This PR aims to achieve the above. Note that I'm not a seasoned typescript developer, and I had some issues with yarn in this repo, so I had to bypass the pre-commit hook.

@michaelbromley
Copy link
Member

Hi,

thanks for the PR! I'm in favour of allowing control over this, but I am hesitant to change the default right now. As far as I understand, the tradeoff with the serializable level is that it is more expensive and can also have an effect on the number of deadlocks encountered. I'd rather have this as opt-in at least to start with until we can gather some real-world experience of any side-effects this change might have.

By the way, did you run into any anomalies due to the isolation level when working with Vendure so far?

@domsj
Copy link
Contributor Author

domsj commented Apr 12, 2023

Hi,

thanks for the PR! I'm in favour of allowing control over this, but I am hesitant to change the default right now. As far as I understand, the tradeoff with the serializable level is that it is more expensive and can also have an effect on the number of deadlocks encountered. I'd rather have this as opt-in at least to start with until we can gather some real-world experience of any side-effects this change might have.

I see, I understand your hesitation. And I see the failing tests (specifically related to this).

I would expect the performance tradeoff to be okay, but the potential deadlocks are a bit annoying (even if after some time the database will detect them). I think the deadlocks can be avoided depending on how the sql is written, but that might work a bit against typeorm, and would require some changes here and there, so not something to just change quickly.

By the way, did you run into any anomalies due to the isolation level when working with Vendure so far?

I ran into an anomaly when adding some custom code. A test that verified concurrent behavior of the update noticed that 2 threads were able to successfully update some object while only 1 should've been able to.
So no Vendure specific faulty code to point to (at least not at this time).
I would prefer not needing to think about anomalies though 😰, hence my preference stated above.

I updated the PR

Copy link
Member

@michaelbromley michaelbromley left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Just a small adjustment to the docs needed then we're good to go 👍

packages/core/src/api/decorators/transaction.decorator.ts Outdated Show resolved Hide resolved
@domsj domsj requested a review from michaelbromley April 15, 2023 10:46
@michaelbromley michaelbromley merged commit bf2b1f5 into vendure-ecommerce:minor Apr 19, 2023
@michaelbromley
Copy link
Member

Thank you for your contribution!

@domsj domsj deleted the isolation branch April 19, 2023 16:43
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.

2 participants