Re: [PATCH] Convert LocalStatus to sqlite

[Stewart Smith <stewart@xxxxxxxxxxxxxxxx>]

On Wed, 2007-03-28 at 10:29 -0500, John Goerzen wrote:
> On Wed, Mar 28, 2007 at 10:03:38PM +1000, Stewart Smith wrote:
> > Forgive the potentially bad python, not my native tongue :)
> 
> Hi Stewart,
> 
> Thanks for the report, idea, and especially for backing it up with code.
> 
> Before applying, however, I want to make sure that I have a good
> understanding of the underlying problems and of your fix.
sure.

> > This patch is motivated by three things:
> > - offlineimap is extremely slow at syncing lots of locally deleted
> > messages
> > - offlineimap uses lots of memory
> 
> Regarding these two points, I have never seen these problems except when
> using one of the GUIs (which I will be removing from OfflineIMAP
> anyhow).  Can you provide some details on your mailbox sizes and setup?

I use the TTY ui - found it the most reliable.

All in all, I think it's about 8-9GB of mail (2.5GB in a .tar.bzip2
archive that i create out of cron for backup).

Server is courier-imap serving out of Maildir. Server is Sempron 2800,
1GB memory. Disk is RAID1 running XFS.

Client is laptop, 5400rpm disk, XFS, 2.13ghz Pentium M, 2GB memory.

about 153 mailboxes. Largest is about 170,000 messages... at least
another a bit over 100,000. 5-10k messages fairly common. 30k messages
in a few folders.

The typical test was deleting  about 20,000 messages from a 120,000
message folder.

> > - LocalStatus files aren't written safely (a hard crash can cause
> > corruption)
> 
> Please explain how this is so.
> 
> I went back and double-checked the code and it looks completely safe to
> me.  The new data is written only after a folder syncs properly.  It is
> written to a temporary file.  That file is then closed, and only if all
> of that worked fine, it is renamed on top of the old file.

problem is that neither clone or rename imply sync. So clever systems
can have the rename hit disk before the data in the file does.

a fsync before the close would fix this.

> If there is a problem at any point, you should have either the old or
> the new data intact.  Believe me, I've had plenty of OfflineIMAP
> exceptions in my time and have not had trouble with this ;-)

so have i :)

exceptions aren't really the problem... hard machine crash is where
you'll see it - and very very very rarely on ext3 with data=ordered
journalling mode - so for a lot of installs it's really hard to hit.

> The only thing I can spot is that maybe we ought to fsync() the
> individual message files when wrotten before closing the status file.

that too. been bitten by that as well.

> That would be a trivial patch.  I'll write it up shortly.

great.
> 
> >     - I've been bitten by this in the past, causing a complete resync of
> > the folder... so I get duplicate messages.
> 
> And this could happen with your patch as well, if a message is synced
> and stored to disk before the corresponding status is committed.  I do
> not believe your patch would fix this.

You'd get a duplicate of that single message, not the entire folder - as
I have done in the past. This is due to the not sync before rename
described above.

> > In my tests, execution time for a normal sync is relatively the same.
> > 
> > Execution time for when lots of messages have been deleted in a
> > reasonably sized folder (e.g. during re-organisation of mail folders) is
> > as much as 10x faster.
> 
> Can you post the output from time(1) on the same operation with the old
> approach and the new?  Also, how many messages were in the box before
> the delete, and how many after?

I tested like this (i'll be able to get more numbers when i fix the
bootloader problem on another machine that has enough disk to do
reasonable amounts of syncs):

- make a copy of local status file for folder.
- sleep 1
- diff copy with current.

typical difference would be 1 to 2 messages.

with my patch:
- select count(id) from status;
- (and repeat)

typical would be greater than 10/sec.

(this was for the ~20k removed from ~120k folder)

I changed the patch since that number to not commit to the db on every
operation... I believe this is safe, as on a crash of the machine we'd
just run the sync again and realise we need to remove things from
localstatus and retry the txn.

If we can also do this for updating flags, we can get a heck of a lot
better performance on updating flags - currently it commits to the DB
for every update of the flags (placing a large performance limit).

> > I had disable the threading for copying messages as this means that
> > LocalStatus objects are shared between threads, which pysqlite doesn't
> > like (it asserts).
> 
> I noticed that.  I will not be applying this patch until that is fixed
> for sure.  Losing threading would be a significant regression for a lot
> of people for the benefit of one particular isolated case.

Different folders can still run in different threads all okay... it's
just if you want to copy messages in the same folder in parallel (unless
i'm misunderstanding the code).

Should be okay if we have the thread create a new instance of the
LocalStatus object though. sqlite itself will then get locks at the
appropriate time and the python code should be all okay.

thoughts?


> > A future patch may convert other storage types to sqlite (or similar) to
> > further reduce memory consumption (and hopefully runtime).
> 
> A couple of other nits:
> 
> * Your patch doesn't apply cleanly to the source tree as it exists in
>   Darcs.

ahh.. was hoping that wasn't the case... 

> * There appear to be some indentation problems (which may be related
>   to the patch not applying cleanly)  This will likely lead to trouble.
>   These exist in your new version too, but also here...

possibly due to a very poorly set up editor too. Pretty sure my emacs
has absolutely no modes for python. Any recommended ones?

> 
> >  class LocalStatusFolder(BaseFolder):
> > +    def __deinit__(self):
> > +   self.save()
> > +        self.cursor.close()
> > +   self.connection.close()
> > +
> 
> That's one example.
> 
> > -            os.unlink(self.filename)
> > +       self.cursor.close()
> > +       self.connection.close()
> > +            os.unlink(self.dbfilename)
> 
> Here's another.

cool.... no doubt editor strangeness.

> Once again, thanks much for the submission!

no worries - hope we can sort the things out and get it in. Thanks for
the feedback.


-- 
Stewart Smith (stewart@xxxxxxxxxxxxxxxx)
http://www.flamingspork.com/


-- Attached file included as plaintext by Ecartis --
-- File: signature.asc
-- Desc: This is a digitally signed message part

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)

iD8DBQBGCo/dKglWCUL+FDoRAr6SAKC+vzgjdjItm/k/I5/QBSeJykolmQCeP22B
iRpMFqZmD6a+YKH+ccTVHnE=
=pwLV
-----END PGP SIGNATURE-----