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

Standardize transaction isolation #10275

Open
jonasraoni opened this issue Aug 5, 2024 · 6 comments · May be fixed by #10276 or pkp/ojs#4386
Open

Standardize transaction isolation #10275

jonasraoni opened this issue Aug 5, 2024 · 6 comments · May be fixed by #10276 or pkp/ojs#4386
Assignees
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days.
Milestone

Comments

@jonasraoni
Copy link
Contributor

Describe the bug
Laravel doesn't setup a default transaction isolation, then things might behave differently depending on the default isolation set at the database configuration.

The default transaction isolation of MySQL is repeatable read, while PostgreSQL uses read committed. A sysadmin can also change the defaults, even though it's unlikely to happen, we shouldn't trust on it.

We should go with the read committed, it represents better what's happening at the database, is more performatic (the database won't have to manage too much data/issue extra locks), and by reducing the number of locks, the occurrence of deadlocks should also decrease.

The difference lies on the possibility of phantom reads, which is ok!

What application are you using?
OJS 3.3

@jonasraoni jonasraoni added the Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. label Aug 5, 2024
@jonasraoni jonasraoni added this to the 3.5 Internal milestone Aug 5, 2024
@jonasraoni jonasraoni self-assigned this Aug 5, 2024
jonasraoni added a commit to jonasraoni/pkp-lib that referenced this issue Aug 5, 2024
@jonasraoni
Copy link
Contributor Author

While I was writing a discussion, I've noticed the isolation isn't stable. I'll update just the main branch. Anything against @asmecher?

@asmecher
Copy link
Member

asmecher commented Aug 9, 2024

I'm ambivalent... I would prefer we hewed close to the defaults in general, and I don't think this solves a problem anyone's experienced.

@jonasraoni
Copy link
Contributor Author

I think we need to enforce it for the sake of predictability. In theory changing the default of MySQL should improve the performance when under higher concurrency. The market standard is to use the read committed isolation (MySQL is an outlier).

In one of the discussions, I proposed an idea that would need a specific isolation to work properly (otherwise the idea would work fine in PostgreSQL, but not in MySQL).

@asmecher
Copy link
Member

@jonasraoni, what's the feature that requires isolation?

@jonasraoni
Copy link
Contributor Author

jonasraoni commented Aug 13, 2024

Ah, I've left a note on the "Drawbacks" of #10274. If the system is under a transaction, and you want to have another independent operation(s), then you need a separate transaction/connection. And if you want both to see each other committed changes, a read committed is needed.

IMHO the behavior of repeatable read produces a fake reality (the data is on the database, already committed, but you may or may not see it, depending on your previous queries) and it has extra costs/locks.

The other point that I care, is to have an environment as standard as possible. So, we should either use read committed or repeatable read, but not leave it to fate.

@asmecher
Copy link
Member

We haven't started using transactions yet (and we're not ready to start just yet -- there's a relevant discussion in the Development channel in Mattermost from 2024-07-31) and #10274 isn't on the roadmap yet, so I don't think this change is motivated just yet. I am motivated by a reduction in locks, but that won't materialize until we start using transactions. Could you turn this into a discussion and scope it up to be a general capture on transactions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days.
Projects
None yet
2 participants