Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Data/SQLite in develop does not support automatic retry #248

Open
obiltschnig opened this Issue · 13 comments

3 participants

@obiltschnig
Owner

This was implemented in 1.3.4 and was available up to 1.4.6, but has not been integrated to develop. While at it, also check that all other SQLite improvements added in 1.3.x and 1.4.x have been integrated to develop.

See also: http://stackoverflow.com/questions/17863464/retries-with-locked-sqlite-databases-in-poco-1-5-x

@obiltschnig obiltschnig was assigned
@aleks-f aleks-f added the enhancement label
@cristiantm

Is someone working on this? If not, I might take a look on it. It affects my project.

@obiltschnig
Owner
@cristiantm

Ok!

I´m working on it on my fork. I´m doing a manual merge of the 1.4.6 tag. I think I have it all done already, but will need more time to check if I did not missed something and also implement some tests, that did not existed for that feature on the 1.4.6 anyway.

If anyone wants to see, comment or colaborate:
https://github.com/cristiantm/poco/tree/enh-sqlite-lockedRetry

@cristiantm

Note to self: Have a look on this too, since its related, and maybe fix it too: #483

@cristiantm

I´m looking better into this I believe this should NOT be ported. Look at this:
http://www.sqlite.org/c3ref/busy_timeout.html

I think setting a higher timeout (we actually have this on the API, calling setConnectionTimeout) makes sqlite do what we are trying to do here internally. So no reason to deal with this at all on Poco. Anyone against it?

@obiltschnig
Owner

I'm OK with that. Can you verify that it indeed does the expected?

@cristiantm

It does work. When set, the Exception thrown on case of SQLBusy happens only if the db is still locked after the timeout set. I´ll take a look on the sqlite code itself but from the description and behaviour it does just exactly what poco was doing.

@cristiantm

I´ve looked at the code. It actually does something very similar than Poco was doing. But it does not waits a random time, it starts sleeping a very short time (1ms), that is a little bit bigger until 100ms, and after that it starts waiting 100ms every round. Other than that, it works as Poco was doing.

Maybe should we just document this? Or maybe add the old setMaximumSleep and minimumSleep and mark them as deprecated and make them a wrapper for the sqlite busy timeout?

One problem I noticed, if I did not missed anything, is that we currently can not set this timeout to all sessions in a SessionPool. Any suggestion on that?

@aleks-f
Owner

Session and SessionPool features and properties should be set via setFeature(const std::string&, bool) and setProperty(const std::string&, const Poco::Any&)

@cristiantm

Yes, I understand that. But the timeout that we set using setConnectionTimeout is not currently a property or feature.

Maybe we could make it be a property (a property that is a wrapper for setting sqlite_busy_timeout). One could suggest to use the old maxSleep, minSleep and max retries, but we actually do not have all this properties, we just set maxSleep on sqlite_busy_timeout.

So one suggestion would be to have maxSleep to be a wrapper to setting timeout, and mark the other two as deprecated. While this would allow to set timeout on session pools, it would give us two ways of doing the same thing from the SQLite session perspective.

@aleks-f
Owner

I would just make "connectionTimeout" a property. In order to be usable from SessionPool,this property should be registered for all backends in their respective constructors; I think it is generic enough for that approach.

@cristiantm

Uhm... looks ok. Something like the following, that should now be ported to all other backends that have timeout (probably all). While there are now two ways for doing the same thing (use setConnectionTimeout method or using the connectionTimeout property), we at least keep backward compatibility by doing it that way.

https://github.com/cristiantm/poco/tree/enh-sqlite-timeoutProperty

@cristiantm

I´ll submit this as is as a pull request, since it works for sqlite. I wont have time to expand it to all other backends and specially test it on them, but looks fine to have different available properties for different backends. If anyone wants to follow and expand the timeout property support on other backends can do it later on. Looks ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.