385834 - don't do incremental vacuuming, makes fragmentation worse [was: places' sqlite file can get overly large, do incremental vacuuming]

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

we should investigate incremental or periodic vacuuming.

Comment 1

[(not reading, please use [e-mail] instead)]

we used to do vacuum on in nsNavHistory::RemoveAllPages() but we appeared to comment it out for performance reasons.

see:

http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#2534

and bug #328598

Comment 2

[Dietrich Ayala (:dietrich)]

 per sdwilsh: do full vacuum using nsIIdleService.

Comment 3

[Jo Hermans]

see also bug 383031

Comment 4

[Jo Hermans]

and bug 334010

Comment 5

[(not reading, please use [e-mail] instead)]

moving to m8.  but see also bug #390244

Comment 6

[(not reading, please use [e-mail] instead)]

taking as this blocks #390244

chatted with dietrich, and our plan is:

1)  "use PRAGMA auto_vacuum = 2"
2)  on an idle timer call "PRAGMA incremental_vacuum(N)"

I am not sure doing a full vacuum on an idle timer (see comment #2, from sdwilsh) is a good idea, as this is slow and could easily block our UI (see nsNavHistory::RemoveAllPages())

Comment 7

[(not reading, please use [e-mail] instead)]

from http://www.sqlite.org/pragma.html

Auto-vacuuming is only possible if the database stores some additional information that allows each database page to be traced backwards to its referer. Therefore, auto-vacuuming must be turned on before any tables are created. It is not possible to enable or disable auto-vacuum after a table has been created.

so, this means that our existing users will not benefit, until we have another migration.

but fix in hand for new users.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: patch, need to improve the timeouts and page size before seeking a review

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: updated patch to trunk, still need better timeouts and page size

Comment 10

[(not reading, please use [e-mail] instead)]

additionally, for existing users (before the incremental vacuum), what about looking for a long idle and doing a full vacuum?

Comment 11

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

if the user has been idle for 15 minutes, do an incremental vacuum if the database has been set up for incremental vacuuming.

we might consider turning on the if 0 code to do a full vacuum, but that can be slow and block the UI.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: updated patch

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 278537 [details] [diff] [review]
updated patch

>+    // if our database was created with incremental_vacuum, 
>+    // do incremental vacuuming
>+    if (vacuum == 2) {
>+      rv = mDBConn->ExecuteSimpleSQL(
>+        NS_LITERAL_CSTRING("PRAGMA incremental_vacuum;"));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+    else {
>+#if 0
>+      // Currently commented out because compression is very slow
>+      rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("VACUUM;"));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+#endif

please file a followup bug for this, if there isn't one already. r=me otherwise

Comment 14

[(not reading, please use [e-mail] instead)]

fixed.

Checking in nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.157; previous revision: 1.156
done
Checking in nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHisto
ry.h
new revision: 1.93; previous revision: 1.92
done

Comment 15

[(not reading, please use [e-mail] instead)]

> please file a followup bug for this, if there isn't one already.

enabling that code would fix bug #390244, so let's take the discussion to that bug.

Comment 16

[(not reading, please use [e-mail] instead)]

from comment #7, I wrote:

"so, this means that our existing users will not benefit, until we have another
migration."

that is incorrect.   migration involes dropping tables, and to switch to incremental vacuum, we need to do it before any tables are created.

so, for all existing alpha users, they won't get incremental vacuuming.  but bug #390244 tracks what we can do about those users.

Comment 17

[(not reading, please use [e-mail] instead)]

for the record, I put the "pragma auto_vacuum=2" call in the wrong spot.

I should have put it after the pragma call to set the page size.

see bug #401985 comment #5 for more details.

Comment 18

[(not reading, please use [e-mail] instead)]

just talked to dr richard hipp, and he suggests that we don't do auto vacuuming / incremental vacuuming at all, because it can actually make fragmentation worse.

for performance, we care more about fragmentation then the size of places.sqlite on disk.

if we care about places.sqlite on disk, vacuum is what we want.

for progress during vacuum, notice we have sqlite3_progress_handler() (and the moz storage wrapper, see http://lxr.mozilla.org/seamonkey/source/storage/public/mozIStorageConnection.idl#250)

a full vacuum gets turned into many sqlite virtual machine op codes, so we'll get progress (but we won't know how long it will take, so we would need cyclon / indeterminate progress mode.)

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: disable incremental vacuuming

Comment 20

[(not reading, please use [e-mail] instead)]

re-opening bug.  I'd like to back this out, after talking to dr. hipp.

auto vacuuming was designed when disk space is at a premium, as full vacuum (which will defrag) requires disk space for the temp vacuum db that it creates.

auto-vacuum is good when fragmentation doesn't hurt you, like when your .sqlite database is not on disk (in memory on a device)

using auto-vacuum will cause us to fragment more than not using it, which will negatively affect performance.

note, places.sqlite will never shrink (unless you vacuum), as it re-uses freed pages.  but having a big places.sqlite won't hurt us, as only "valid" db pages use up the db cache.

note, for users who have places.sqlite created with pragma auto_vacuum=2 (incremental, not auto), will be ok, as long as we don't call incremental_vacuum.

 

Comment 21

[(not reading, please use [e-mail] instead)]

morphing bug.

Comment 22

[(not reading, please use [e-mail] instead)]

Comment on attachment 287060 [details] [diff] [review]
disable incremental vacuuming

dietrich has a better idea, keep the timer mumbo jumbo, but just remove the vacuum stuff (and rename it), as he has other uses for a places idle timer.

Comment 23

[(not reading, please use [e-mail] instead)]

over to mr. d

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: backout only incremental vacuum

- generalize the idle timer, so that expiration patch in bug 332748 can hook into it
- remove incremental vacuum

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 287067 [details] [diff] [review]
backout only incremental vacuum

r=sspitzer, but remove these two lines:

+    // if our database was created before incremental vacuuming
+    // do a full vacuum on idle

Comment 26

[(not reading, please use [e-mail] instead)]

we might also want to change:

+    // Currently commented out because compression is very slow
+    // see bug #390244 for more details

to be:

+    // Currently commented out because vacuum can be slow
+    // see bug #390244 for more details

thanks again, dietrich.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 287067 [details] [diff] [review]
backout only incremental vacuum

a=endgame drivers for M9

Comment 28

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.182; previous revision: 1.181
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.109; previous revision: 1.108
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.183; previous revision: 1.182
done

Comment 29

[Marcia Knous [:marcia]]

Dietrich: QA could use some help with steps to verify this bug. Thanks.

Comment 30

[Dietrich Ayala (:dietrich)]

Hey Marcia, with a new profile:

1. Open SQLite Database Browser (or the cli, or any sqlite querying tool)
2. Execute the following SQL: "pragma auto_vacuum"
3. Confirm that the value returned is zero

For existing profiles, auto_vacuum will have a value of 2, but we removed the code that actually initiates the vacuuming. Maybe you could wait until 15 mins of idle time, and confirm that your places.sqlite file's size and last modified time doesn't change.

Comment 31

[Marcia Knous [:marcia]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110304 Minefield/3.0a9pre. I verified using the steps in Comment 30.

Comment 32

[Justin Kerk]

(In reply to comment #20)
> note, places.sqlite will never shrink (unless you vacuum), as it re-uses freed
> pages.  but having a big places.sqlite won't hurt us, as only "valid" db pages
> use up the db cache.

Does this impact the effectiveness of the "clear private data" feature?

Comment 33

[(not reading, please use [e-mail] instead)]

> Does this impact the effectiveness of the "clear private data" feature?

Justin, if you're worried about your data being left behind in the sqlite database (because we aren't vacuuming), then we are ok.

we compile sqlite with the SQLITE_SECURE_DELETE compile-time option, which causes deletes to overwrite old data with zeros.

See http://lxr.mozilla.org/mozilla/source/db/sqlite3/src/Makefile.in#78

Or did I misunderstand your question?

Comment 34

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 35

[Lionel Elie Mamane]

(In reply to comment #33)
>> Does this impact the effectiveness of the "clear private data" feature?

> Justin, if you're worried about your data being left behind in the sqlite
> database (because we aren't vacuuming), then we are ok.
 
> we compile sqlite with the SQLITE_SECURE_DELETE compile-time option, which
> causes deletes to overwrite old data with zeros.

That's not true, or not true anymore. I just found references to websites of my browse history in places.sqlite (with "grep"), after clearing private data, that were not in the result of .dump and that disappeared after a VACUUM.

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> That's not true, or not true anymore. I just found references to websites of my
> browse history in places.sqlite (with "grep"), after clearing private data,
> that were not in the result of .dump and that disappeared after a VACUUM.
These comments aren't helpful if you don't include your user agent string...

Comment 37

[Lionel Elie Mamane]

(In reply to comment #36)
> (In reply to comment #35)
> > That's not true, or not true anymore. I just found references to websites of my
> > browse history in places.sqlite (with "grep"), after clearing private data,
> > that were not in the result of .dump and that disappeared after a VACUUM.
> These comments aren't helpful if you don't include your user agent string...

It seems to be a Debian-specific issue; sorry for the noise. In the Debian packages, the bundled sqlite is not used, but xul is linked against the system sqlite, which is not compiled with SQLITE_SECURE_DELETE. I filed Debian bug #566326 for this.

Comment 38

[Shawn Wilsher :sdwilsh]

(In reply to comment #37)
> It seems to be a Debian-specific issue; sorry for the noise. In the Debian
> packages, the bundled sqlite is not used, but xul is linked against the system
> sqlite, which is not compiled with SQLITE_SECURE_DELETE. I filed Debian bug
> #566326 for this.
Right, and if they were running our unit tests, they would have caught this.  For what it's worth, we recently added a configure check for this as well.