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 MaybeTransaction for better writing speed when supported #584

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Atreyagaurav
Copy link
Contributor

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Fixes #582

@Atreyagaurav Atreyagaurav marked this pull request as ready for review November 2, 2024 00:22

impl Dataset {
pub fn maybe_start_transaction(&mut self) -> MaybeTransaction<'_> {
let force = 1;
Copy link
Member

Choose a reason for hiding this comment

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

We should add at some point that parameter for force, but if you want this for better performance, you absolutely don't want to enable it.

I think it only works for FileGDB, and it's implemented by making a file system-level copy of the whole dataset, and restoring it on rollback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that won't help with performance, I think if we want transaction for rollback purposes vs want transaction for speed purposes, I think we should have different methods.

The MaybeTransaction doesn't have rollback method at all, that'd help at compile time to let people know they can't rollback. Since it could be working on the database directly without transaction as well.

That's why I was thinking maybe some other name would be better. The current interpretation of MaybeTransaction is it is maybe a transaction or not, hence it has to be committed on drop, and no rollback. To make the Transaction and non transaction database do same thing.

On that note I think we can add the force thing to normal Transaction or add another ForceTransaction that does filesystem copy ourselves when Transaction fails.

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.

[Feature Req] Optionally use Transaction if supported
2 participants