The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 8

Status

()

Toolkit
Places
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug, {verified-aurora, verified-beta})

Trunk
mozilla10
verified-aurora, verified-beta
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8 fixed, firefox9+ fixed)

Details

(Whiteboard: [qa!])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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.
Attachment #564561 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #564561 - Flags: review? → review?(dietrich)
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
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+
(Assignee)

Comment 3

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a98d668e0c
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
(Assignee)

Comment 4

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Comment 6

6 years ago
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?
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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

6 years ago
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+
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e86daeba6c5
status-firefox9: --- → fixed

Comment 12

6 years ago
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

6 years ago
(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

Updated

6 years ago
Attachment #564561 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 14

6 years ago
This doesn't seem to just transplant to beta...can you please land it there?
(Assignee)

Comment 15

6 years ago
sure, I think I may already have the beta patch ready locally.
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/b2f7b1fac9fb
status-firefox8: --- → fixed
tracking-firefox8: ? → ---

Updated

6 years ago
tracking-firefox9: ? → +
Is there anything specific QA can do to verify this is fixed?
Whiteboard: [qa?]
(Assignee)

Comment 18

5 years ago
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?
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
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.
(Assignee)

Comment 24

5 years ago
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.