Skip to content

Enable basic RDF transaction isolation (was none)#304

Draft
mirzov wants to merge 1 commit into
masterfrom
transaction_isolation
Draft

Enable basic RDF transaction isolation (was none)#304
mirzov wants to merge 1 commit into
masterfrom
transaction_isolation

Conversation

@mirzov
Copy link
Copy Markdown
Contributor

@mirzov mirzov commented Mar 26, 2025

No description provided.

@mirzov mirzov requested review from ggVGc and jonathanthiry March 26, 2025 13:13

def transact(action: RepositoryConnection => Unit): Try[Unit] = transact(action, None)
def transact(action: RepositoryConnection => Unit): Try[Unit] =
transact(action, Some(IsolationLevels.READ_COMMITTED))
Copy link
Copy Markdown
Contributor

@ggVGc ggVGc Mar 27, 2025

Choose a reason for hiding this comment

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

I think we should set some isolation level (otherwise why use transactions at all), but why are we choosing READ_COMMITTED specifically? I would prefer to always use the strongest isolation level possible as default, unless we hit practical limitations, since resource usage is easier to fix than data corruption (even though in our case we have rdflog, so we are still fairly safe).
Even if we do, I think we should still use SERIALIZABLE as the default, and adjust usages of Repository.transact where we hit performance issues to a weaker isolation level, while also making sure that the code within that transaction actually is not expecting a higher level.

I would maybe even prefer that we don't have a default isolation level at all, since that choice should depend on the logic inside the transaction, but I think defaulting to SERIALIZABLE is still okay.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, good question. I would think that READ_COMMITED should be good enough for our use case, but it could also be worth to review the transact call sites.

@ggVGc ggVGc marked this pull request as draft September 24, 2025 13:03
@ggVGc
Copy link
Copy Markdown
Contributor

ggVGc commented Sep 24, 2025

Converted this to draft since it still requires some decisions regarding which transaction level to use. Also, if we move to a relational DB, this issue is not relevant.

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