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

Retry on SQLITE_BUSY since, this means sqlite is busy and cannot #189

Closed
wants to merge 1 commit into from

Conversation

sriramster
Copy link
Contributor

The SQLITE_BUSY by definitions means the write transactions is not possible right now cause the db lock is not available. And a retry for the sqlite transaction is ok.

Since we do not use transactions yet, which has an option to BEGIN_IMMEDIATE to partially avoid this problem. Right now a retry is ok.

@Karsten1987 Please let me know if this makes sense.

Thanks for your time in reviwing the patch.

acquire a lock more than a failure on i/o
@Karsten1987
Copy link
Collaborator

I think throwing an exception immediately might be maybe a bit extreme, however your approach, I feel, is kind of the other extreme.
I could see that your approach could potentially lead to a busy wait loop.

@sriramster
Copy link
Contributor Author

True,

But sqlite3 gurantees it does'nt happen. So I moved here. Could you suggest?

@Karsten1987
Copy link
Collaborator

Karsten1987 commented Oct 25, 2019

@sriramster can you point me to a piece of documentation where I could read a bit more abut that behavior?
From the SQLite documentation:

For example, if process A is in the middle of a large write transaction and at the same time process B attempts to start a new write transaction, process B will get back an SQLITE_BUSY result because SQLite only supports one writer at a time. Process B will need to wait for process A to finish its transaction before starting a new transaction. The sqlite3_busy_timeout() and sqlite3_busy_handler() interfaces and the busy_timeout pragma are available to process B to help it deal with SQLITE_BUSY errors.

I believe there are some timeout callbacks we can set to avoid constantly querying if the database is still busy. The way I see you implementation at the moment results in a busy loop constantly re-trying to call step until the database is not busy.

@sriramster
Copy link
Contributor Author

@Karsten1987 I think so, thanks for reviewing this. I'll change this up to support a callback function which could serve the purpose than retry-ing.

My reference of retrying loop was this URL https://activesphere.com/blog/2018/12/24/understanding-sqlite-busy

You may close this pull request. I'll reopne when I have something concrete.

Thanks for the time again.

@sriramster sriramster closed this Oct 26, 2019
@Karsten1987
Copy link
Collaborator

Thanks for looking into this. I also feel you've got quite some insights in SQLite3. From your perspective, how hard is it to actually refactor the sqlite plugin to use transactions?

@sriramster
Copy link
Contributor Author

Hi @Karsten1987

I've a working version of the transactions, but it needs structuring and cleaning up todo. This is what the idea on paper looks like

https://gist.github.com/sriramster/2f9bdd620730855e6172c7136b56d6fc

Let me know your thoughts.

Thank you.

@sriramster
Copy link
Contributor Author

And, with transactions the perfomance increased (only inserts) with some of my bags collected to atleast 25% (positively)

@sriramster
Copy link
Contributor Author

Hi @Karsten1987 Have you had time to look at the idea?

@Karsten1987
Copy link
Collaborator

the diagram looks like an interesting starting point. Increasing the cache size could potentially also be exposed as a custom parameter.
From your previous comments, I take it you already have code for this in place? If so, feel free to open a PR for it and we iterate from there.

@jacobperron
Copy link
Member

@sriramster Friendly ping. Any update on this?

@sriramster
Copy link
Contributor Author

Hi @jacobperron

#225

We are working on the transactions in the above pull request.

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