[PATCH 5/7] Re: Don't keep sqlite connections open

[Sebastian Spaeth]

On Fri, 6 May 2011 00:34:39 +0200, Nicolas Sebrecht wrote:
> >      def upgrade_db(self, from_ver):
> >          """Upgrade the sqlite format from version 'from_ver' to current"""
> 
> This is a good improvement but...

Thanks

> 
> ...we tend to have a lot of constructs like this
> 
>   conn, cursor = self.get_cursor()
>   with conn:
>     while True:
>       try: 
>         <execute some SQL request and commit>
>       except blablah:
>         <retry on database lock>
>       else:
>         bail out of the while loop

Yes, 4 times or so, as sqlite will simply bail out if another thread
currently locks the db, rather than waiting for the lock.

> it would be nice to factorize out. Not that this is wrong by design but
> it will cause harder work to maintain with possible errors and bugs.

I agree that factoring that out would be nice, but... ( :-) )
... I really don't want to put that into this topic series. I have been
spending plenty of time in refining each of the patches (always doing
'git rebase -i next'), and the series is already as big as I am
comfortable with a series.

How about we think about merging this as is now, and work on a followup
series that builds on what we have and factors out whatever is
possible. For example, we still don't know if this approach [opening and
closing the db for each transaction] is scalable at all or whether we
don't need a "db owning thread" that we pass the data to. I'd rather
gather some data from others first.
Alternatively, it got pointed out that *newer versions* of sqlite can
access the db from multiple threads. I would love to find out *how new*
that version must be and improve our design to not always close and open
our database first before embarking on more architecture design.

> I think we need a wrapper (or more) proceeding the SQL requests with
> generic data methods. Let's me try to categorize the different request
> types:

>   - read only: mostly SELECTs with no need to commit()

right, no need to check for locks here, as we won't conflict. (Well
after reading the docs, it basically states that one must always expect
them)
 
>   - write (we need to commit()):
>     * one statement: easy
>     * multiple statements: easy using a list

Yes.

>     * statements including additional code to execute: pass both the
>       request and a function to the wrapper

Mmmh, I am not really sure I get this yet, can you give an example? What
additional code could you be thinking off? Are you thinking of something like

def savemessageflags(UID, flags):

 res = data.write(SQL     ='UPDATE status flags=:flags WHERE UID=:UID',
                  sqlargs ={UID:1,flags:'T'},
                  func=<a function that does something (with what?)>)
 if res: ....
>   - statistics
>   - advanced tracking system for logging IMAP/cache sessions
>   - manage miscallenous exceptions
>   - multiple data formats as backend (usefull for testing purpose ,-))

The above 4 we can already do with quite less effort I think, see how
*'relatively easy'* it was to plug in a new data format as
backend. Improved exception handling will come anyhow now that a
foundation for them is in place.

>   - thread safe concurrent access by managing queues

Right, that would be an advantage, but we need to take care that we make
sure we don't continue before the queue has been worked off. Offlineimap
takes care that it can "crash" anytime without losing data, so we
shouldn't just stuff queries in a queue and continue. Not insovable, but
requires some design thinking.

That is why I would love to get what we have as experimental and
optional in the code now and go on improving it from there.

> I think the SQL topic is a very good opportunity for improving our data
> driver design. In short, I would expect something like data.write(),
> data.read(), (...).

mmmh, I would think that the current system already provides quite a bit
of abstraction by providing overridable classes for each backend for
each functionality, but I would be happy to get a discussion started on
what and how to improve things. But I still feel that is someout out of
the scope of this topic series.

Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20110506/2581faea/attachment-0001.sig>