637352 - purge service shouldn't open db's if there are no retention settings

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

Attachment: proposed fix

We store per-folder override of a server's retention settings in the msg database for the folder. When the purge service is applying retention settings, it causes the db to get opened to check if the folder has overridden the retention settings. Since this is relatively rare, it would be better to store a folder property that says whether the folder is using the server retention settings - if so, we don't have to open the db. This helps in the case of a user with enormous folders w/o retention settings, when opening a db can block the ui for seconds.

I can't write a unit test for the purge service, since it's timer-based and we don't have prefs for the intervals, but I can probably write a unit test for the retention settings.

Comment 1

[David :Bienvenu]

I can verify that for my profile, the purge service no longer opens db's. This would cut down on general disk churn and heap use.

Comment 2

[Wayne Mery (:wsmwk)]

iirc you said the purge interval is 5 minutes?

Comment 3

[David :Bienvenu]

(In reply to comment #2)
> iirc you said the purge interval is 5 minutes?

It is, but we'll only ever purge any given folder every 8 hours.

Comment 4

[David :Bienvenu]

Attachment: fix with unit test

This adds a unit test for basic retention policy handling.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 519551 [details] [diff] [review]
fix with unit test

So there's one main issue I see with this patch - the first time it is run, it leaves virtually all the DBs open. I believe it will also leave DBs open every 8 hours if the folders/servers are set to retain all messages.

I think the issue is that before the GetRetentionSettings call was wrapped by opening the database, doing the call, doing the apply, and then closing the database.

With the new code you're only closing the database if we actually apply the retention settings. 

>+            nsCOMPtr <nsIMsgIncomingServer> incomingServer;

nit: no space before < (several places I think)

>+            if (useServerDefaults)
>+              useServerRetention.AssignLiteral("1");
>+            else
>+              useServerRetention.AssignLiteral("0");

Would the useServerDefault ? "1" : "0" notation be better here?

Comment 6

[David :Bienvenu]

(In reply to comment #5)
> 
> So there's one main issue I see with this patch - the first time it is run, it
> leaves virtually all the DBs open. I believe it will also leave DBs open every
> 8 hours if the folders/servers are set to retain all messages.
If folders are using server retention settings, we'll never open the db (after the very first run with the patch).
> 
> I think the issue is that before the GetRetentionSettings call was wrapped by
> opening the database, doing the call, doing the apply, and then closing the
> database.
Hmm, I'll fix that...

> Would the useServerDefault ? "1" : "0" notation be better here?

If it compiled, it would. But iirc, it didn't :-(

Comment 7

[David :Bienvenu]

Attachment: fix addressing comments

this fixes the closing of the dbs when retention settings aren't applied, and adds a simple test that we're not leaving a db open in that case (first time application of retention settings).

Comment 8

[David :Bienvenu]

fixed on trunk

http://hg.mozilla.org/comm-central/rev/b3fe922d09c8