Closed Bug 583852 Opened 11 years ago Closed 11 years ago

Weave should not be querying on places views

Categories

(Cloud Services :: General, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sdwilsh, Assigned: philikon)

References

Details

Attachments

(2 files, 2 obsolete files)

Views are very slow to select on because they have no indexes.  Views also consume more memory (table has to be created in memory to query from).  Additionally, views are very likely to be going away in bug 552023.
Instead of querying the views we should query the UNION of table + temp table as per https://wiki.mozilla.org/Places/Places_SQL_queries_best_practices. This should also prevent the symptoms of bug 575423. If the temp table doesn't exist, bug 552023 landed and we don't have to worry about this anymore and query the table directly.
Duplicate of this bug: 580825
Blocks: 584481
Attached patch tests (obsolete) — Splinter Review
Tests for the history store.
Assignee: nobody → philipp
Attachment #463910 - Flags: review?(mconnor)
Attached patch v1 (obsolete) — Splinter Review
Query moz_places, moz_historyvisits *and* moz_places_temp, moz_historyvisits_temp instead of the slower views. If the temp tables aren't available (bug 552023), just query the regular tables.

Also fixed some style and syntax nits.
Attachment #463911 - Flags: review?(mconnor)
Attached patch v1.1Splinter Review
Harmonize spelling of table + temp table queries with places:
* query moz_*_temp before moz_*
* use IFNULL rather than IN operator in WHERE clause (potential speedup)
* use UNION ALL instead of UNION in _urlStm() and add LIMIT 1 since there's at most only 1 result row (potential speedup)
Attachment #463911 - Attachment is obsolete: true
Attachment #463922 - Flags: review?(mconnor)
Attachment #463911 - Flags: review?(mconnor)
Attached patch tests v1.1Splinter Review
Make tests work when running as part of xpcshell-tests on mozilla-central. Was missing the UHist directory alias again, moved it into head_helpers.js so that all tests will have it available.
Attachment #463910 - Attachment is obsolete: true
Attachment #463977 - Flags: review?(mconnor)
Attachment #463910 - Flags: review?(mconnor)
Attachment #463922 - Flags: review?(mconnor) → review+
Attachment #463977 - Flags: review?(mconnor) → review+
can anyone shed some light on how this can be verified manually?   Otherwise, i just assume from the bug that it sounds like unit tests here that fail against m-c will suffice when sounding the alarm.
The decision to query views was made to fix bug 575423. However, querying views is slow and they're going to go away with bug 552023 (hasn't permanently landed on m-c yet, but there's there's mak's tryserver build I used to verify and run my unit tests against: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mak77@bonardo.net-6bf09cf997cb/tryserver-macosx/)

So essentially this bug is an alternate (better performing) way to fix bug 575423 and at the same time it prepares us for nightlies once bug 552023 lands.
Tarek, can you please verify if this is fixed with the 1.4.4 extension?
(In reply to comment #10)
> Tarek, can you please verify if this is fixed with the 1.4.4 extension?

sorry, wrong bug comment.  disregard.
Running Fennec with Weave, while syncing, I am seeing warnings of this type:

WARNING: 2 sort operations have occurred for the SQL statement '0xac05b118'.  See https://developer.mozilla.org/En/Storage/Warnings details.: file /home/crowder/mobilla/storage/src/mozStoragePrivateHelpers.cpp, line 138


Executing this statement:

(gdb) call sqlite3_sql(mStatements[i].mStatement)
[Thread 0xb02fdb70 (LWP 13115) exited]
$8 = 0xaba1bd68 "SELECT visit_type type, visit_date date FROM moz_historyvisits_temp WHERE place_id = IFNULL( (SELECT id FROM moz_places_temp WHERE url = :url), (SELECT id FROM moz_places WHERE url = :url) ) UNION SEL"...


I'm wondering how seriously I should take this warning, but also how we're getting behavior that according to the comment (http://hg.mozilla.org/mozilla-central/annotate/97949d240701/services/sync/modules/engines/history.js#l143) should only happen in Gecko < 2.0
Depends on: 602984
You need to log in before you can comment on or make changes to this bug.