Closed Bug 691509 Opened 13 years ago Closed 13 years ago

Run ANALYZE at each schema change (and force a schema change)

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla10
Tracking Flags:
Tracking Status
firefox8 --- fixed
firefox9 + fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: verified-aurora, verified-beta, Whiteboard: [qa!])

Attachments

(1 file)

patch v1.0
(deleted), patch
dietrich
: review+
asa
: approval-mozilla-aurora+
christian
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attached patch patch v1.0 (deleted) — — Splinter Review 
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.
Attachment #564561 - Flags: review?
Attachment #564561 - Flags: review? → review?(dietrich)
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.
Attachment #564561 - Flags: review?(dietrich) → review+
Flags: in-testsuite+
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.
Attachment #564561 - Flags: approval-mozilla-beta?
Attachment #564561 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/87a98d668e0c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
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?
(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.
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 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?
Attachment #564561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
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.
(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
Attachment #564561 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This doesn't seem to just transplant to beta...can you please land it there?
sure, I think I may already have the beta patch ready locally.
tracking-firefox9: ? → +
Is there anything specific QA can do to verify this is fixed?
Whiteboard: [qa?]
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.
Whiteboard: [qa?] → [qa+]
(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?
(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.
(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.
(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.
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.
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.
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.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: