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

Data loss when fetch() fails in-between store() calls #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Data loss when fetch() fails in-between store() calls #5

wants to merge 1 commit into from

Conversation

djurczak
Copy link
Contributor

If we store multiple values in succession (e.g. in a loop) and there is a fetch() call in-between those store() calls that fails we lose all the data that had been added so far.

This happens because in rb_unqlite_raise() (ext/unqlite/unqlite_exception.c) we call unqlite_rollback() unless the UnQLite error code is either UNQLITE_BUSY or UNQLITE_NOTIMPLEMENTED.

Now, since fetch() is a read-only operation I'd argue that we should avoid calling rollback() in case we fail to find an entry.

A basic fix is pretty trivial but I didn't want to submit it yet since you might instead choose to do slightly more extensive refactoring of the exception code.

I added testcases that generate the problem.

@danieltdt
Copy link
Owner

Nice Daniel, we need to do a refactor on this exception code for sure!
I'll use these tests and start to refactor, thanks!

@djurczak
Copy link
Contributor Author

Cool. In my local version I just quickly addedUNQLITE_NOT_FOUND to those errors that don't generate a rollback() call. But as I said, I am not sure if raising an exception is a good behaviour for fetch() so some more extensive work might be necessary. Which is why I didn't post my fix.

@danieltdt danieltdt mentioned this pull request Jul 30, 2013
@larsch
Copy link
Collaborator

larsch commented Aug 3, 2013

Tests merged (c1840ad) and removed the auto rollback on errors (a84c082). To get auto-rollback one could now do this instead:

db.transaction do
  db.store "key", "val"
  db.fetch "xxx"
end

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