Last Comment Bug 691509 - Run ANALYZE at each schema change (and force a schema change)
: Run ANALYZE at each schema change (and force a schema change)
Status: VERIFIED FIXED
[qa!]
: verified-aurora, verified-beta
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Marco Bonardo [::mak] (Away 6-20 Aug)
:
Mentors:
Depends on:
Blocks: PlacesJank
  Show dependency treegraph
 
Reported: 2011-10-03 14:07 PDT by Marco Bonardo [::mak] (Away 6-20 Aug)
Modified: 2011-12-08 09:21 PST (History)
11 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
+
fixed


Attachments
patch v1.0 (10.69 KB, patch)
2011-10-04 08:41 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
dietrich: review+
asa: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-03 14:07:26 PDT
bug 690354 covers the common cases but we have lots of users out with broken analyze results to be fixed.
We may run analyze on schema change and force a fake change to ensure we run once for all users.
Comment 1 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-04 08:41:49 PDT
Created attachment 564561 [details] [diff] [review]
patch v1.0

that would be this (I borrowed part of a patch made by rnewman and part of the patch in bug 683876).
will push to try both analyze patches now.
Comment 2 Dietrich Ayala (:dietrich) 2011-10-06 17:12:54 PDT
Comment on attachment 564561 [details] [diff] [review]
patch v1.0

Review of attachment 564561 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsNavHistory.cpp
@@ +314,5 @@
>      }
>  
> +    /**
> +     * Updates sqlite_stat1 table through ANALYZE.
> +     * Analyzed tables should stay in sync with nsPlacesExpiration.js

for future generations, please be more explicit in this comment. what should stay in sync? the tables being used? eg: if a table is involved in expiration, should also be added to the set here? if that's the case, please also add a note to the other relevant location.
Comment 3 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-07 12:38:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a98d668e0c
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-07 15:33:18 PDT
Comment on attachment 564561 [details] [diff] [review]
patch v1.0

This ensures we run ANALYZE at least once for all users. Part of the work needed to fix the periodic hangs in the UI.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-09 07:29:28 PDT
https://hg.mozilla.org/mozilla-central/rev/87a98d668e0c
Comment 6 Asa Dotzler [:asa] 2011-10-10 14:54:42 PDT
Marco, can you tell us what the impact will be on user experience? Will users be hanged for long periods while the cleanup is happening? When does it happen? For how long? 

Also, is changing the schema version risky? How often do we do that? Will this effect downgrading? For this late in beta, release team would like to understand the risks with schema changes?
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-10 15:06:36 PDT
(In reply to Asa Dotzler [:asa] from comment #6)
> Marco, can you tell us what the impact will be on user experience? Will
> users be hanged for long periods while the cleanup is happening? When does
> it happen? For how long? 

There should be a small additional time required on upgrade to run analyze, that usually involves some milliseconds (> 50ms, so it may be noticeable). Nothing painful, should largely be invisible, anyone using Nightly already went through this today.

> Also, is changing the schema version risky? How often do we do that? Will
> this effect downgrading? For this late in beta, release team would like to
> understand the risks with schema changes?

This schema change is empty, it's just a trick to force analyze on upgrade, the reason is to immediately fix the issue for all users, rather than have to wait for expiration to run. When we bump from v11 to v12 nothing more than analyze happens, on downgrade the schema is bumped down to v11 (nothing else happens), on a further upgrade there is another bump to v12 with another analyze run.
The last schema bump happened in Firefox 4, but this one is pretty safe considered it's empty.
Comment 8 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-10 17:46:35 PDT
oh, and I actually forgot bug 690354 will need this one to work properly, since to figure out if analyze is needed it uses the stats table, but it may not exist (this patch ensures it exists). I may workaround that, but will add some complexity to first check the table exists.
Comment 9 Asa Dotzler [:asa] 2011-10-11 14:47:48 PDT
Comment on attachment 564561 [details] [diff] [review]
patch v1.0

The release team was comfortable taking this on Aurora but still not thrilled about Beta. Do we have any actual perf testing here with databases of various sizes? Is there anything QA can do to add to the confidence?
Comment 10 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-11 16:01:58 PDT
(In reply to Asa Dotzler [:asa] from comment #9)
> The release team was comfortable taking this on Aurora but still not
> thrilled about Beta. Do we have any actual perf testing here with databases
> of various sizes?

On my 120MB database, that is a large case, a full analyze takes about 300ms, I expect common DBs taking about 100ms, unfortunately I don't have telemetry data yet, so I'm mostly basing on old stats collected around 3.0-3.5.
Bug 683876 is still the best solution to avoid analyze costs completely, but there are perf concerns on that too, that I still have to satisfy before proceeding.
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-12 03:48:01 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e86daeba6c5
Comment 12 MasterLeep 2011-10-13 22:02:35 PDT
This bug is so bad for the affected (and there are many, see http://news.ycombinator.com/item?id=3107878) that I'm surprised there is any question whatsoever that it would make beta.  It makes Firefox almost unusable for large numbers of people.

I am actually surprised that you guys are not pulling all nighters to release a 7.0.2 with this fix in there.
Comment 13 christian 2011-10-18 10:17:59 PDT
(In reply to MasterLeep from comment #12)
> This bug is so bad for the affected (and there are many, see
> http://news.ycombinator.com/item?id=3107878) that I'm surprised there is any
> question whatsoever that it would make beta.  It makes Firefox almost
> unusable for large numbers of people.

Data?

> 
> I am actually surprised that you guys are not pulling all nighters to
> release a 7.0.2 with this fix in there.

This has existed for a while...

Also, please read:

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 14 christian 2011-10-18 13:15:05 PDT
This doesn't seem to just transplant to beta...can you please land it there?
Comment 15 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-18 14:50:19 PDT
sure, I think I may already have the beta patch ready locally.
Comment 16 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-18 17:10:57 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/b2f7b1fac9fb
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-01 14:37:49 PST
Is there anything specific QA can do to verify this is fixed?
Comment 18 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-01 14:46:24 PST
To verify manually you may create a new profile and verify sqlite_stat1 table is populated.
Then take a database from Firefox 4, check sqlite_stat1 does not exist or is not populated, update to current version and check the table is populated.
Comment 19 Virgil Dicu [:virgil] [QA] 2011-12-07 05:20:00 PST
(In reply to Marco Bonardo [:mak] from comment #18)
> To verify manually you may create a new profile and verify sqlite_stat1
> table is populated.
> Then take a database from Firefox 4, check sqlite_stat1 does not exist or is
> not populated, update to current version and check the table is populated.

Could you please detail on this?

Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111206 Firefox/11.0a1

STR:
1. Open Firefox with a new profile (Firefox 11, 10 or 9)
2. Open places.sqlite (used SQLite Manager)
3. Check for sqlite_stat1 table and verify it is populated.

The table is not populated for me when following these steps. Am I missing something?
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-07 05:34:48 PST
(In reply to Virgil Dicu [QA] from comment #19)
> The table is not populated for me when following these steps. Am I missing
> something?

Ehr yes, I pointed out at the wrong verification in that case, since I was thinking of future improvements. We update the statistics at schema change (so on update), while on a new profile those are updated asynchronously when we go over a certain threashold, since the database is empty thus stats would just set 0 everywhere.

The check here should just be on upgrade from a previous version, and more specfically on upgrade from FF7 to FF8, thanks for pointing out that my mind was running too much, I'm sorry if you lose time for that.
Comment 21 Virgil Dicu [:virgil] [QA] 2011-12-07 07:01:17 PST
(In reply to Marco Bonardo [:mak] from comment #20)
> (In reply to Virgil Dicu [QA] from comment #19)
> > The table is not populated for me when following these steps. Am I missing
> > something?
> 
> Ehr yes, I pointed out at the wrong verification in that case, since I was
> thinking of future improvements. We update the statistics at schema change
> (so on update), while on a new profile those are updated asynchronously when
> we go over a certain threashold, since the database is empty thus stats
> would just set 0 everywhere.
> 
> The check here should just be on upgrade from a previous version, and more
> specfically on upgrade from FF7 to FF8, thanks for pointing out that my mind
> was running too much, I'm sorry if you lose time for that.

It's no problem, thanks for clarifying this. Just one more question.

Should the sqlite_stat1 table be populated after an upgrade from Firefox 8 to Firefox 9, for example? The table is populated when upgrading from 7 to 8 or 7 to 9, but not from versions equal or higher than 8.
Comment 22 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-07 07:07:15 PST
(In reply to Virgil Dicu [QA] from comment #21)
> Should the sqlite_stat1 table be populated after an upgrade from Firefox 8
> to Firefox 9, for example? The table is populated when upgrading from 7 to 8
> or 7 to 9, but not from versions equal or higher than 8.


Well, if you are on 8 the table is already populated, moving to 9 should just update the data in the table where needed.
So yes, if you are on 8, DELETE FROM sqlite_stat1 and then upgrade to 9, it should be populated again.
Comment 23 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-07 07:07:50 PST
ehr, well but that happens only if there is a schema bump... it's possible 9 didn't have a schema bump, let me check.
Comment 24 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-07 07:08:59 PST
So, as you can see here, the last schema bump was at Firefox 8, the next one is at firefox 11
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Database.cpp#640

updating from 8 to 9 or 10 won't repopulate, updating to 11 will.
Comment 25 Virgil Dicu [:virgil] [QA] 2011-12-08 09:21:41 PST
Thanks!

Mozilla/5.0 (Windows NT 5.1; rv:8.0.1) Gecko/20100101 Firefox/8.0.1
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0 Firefox 9 Beta5
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111205 Firefox/10.0a2
Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111207 Firefox/11.0a1

Verified on Windows XP, 7, Ubuntu 11.10, Mac OS 10.6 by upgrading successively from 7 to 8, 9Beta5, Aurora (10) and Nightly (11).
The sqlite_stat1 table is populated as specified in comment 20 and comment 24.

Note You need to log in before you can comment on or make changes to this bug.