Closed Bug 659740 Opened 9 years ago Closed 9 years ago

Frecency update causes SQL sort warning

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: rnewman, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file)

WARNING: 1 sort operation has occurred for the SQL statement '0x10bb8b578'.  See https://developer.mozilla.org/En/Storage/Warnings details.: file /Users/rnewman/moz/git/alder/storage/src/mozStoragePrivateHelpers.cpp, line 144

Every. Damn. Record.
Marco + sdwilsh: is there a way for us to see what query might be triggering this warning?
NSPR logging is generally the way to go.  You can also change https://hg.mozilla.org/mozilla-central/annotate/5e1ee7f192ad/storage/src/mozStoragePrivateHelpers.cpp#l118 to print the sql string instead of a pointer.
The query culprit is

  SELECT ROUND((strftime('%s','now','localtime','utc') - v.visit_date/1000000)/86400), COALESCE( (SELECT r.visit_type FROM moz_historyvisits r WHERE v.visit_type IN (5,6)  AND r.id = v.from_visit), visit_type), visit_date FROM moz_historyvisits v WHERE v.place_id = :page_id ORDER BY visit_date DESC LIMIT 10

from

  toolkit/components/places/nsNavHistory.cpp:1295

That query gets called for each history record, presumably via a Places API call.
(In reply to comment #3)
> That query gets called for each history record, presumably via a Places API
> call.
Shouldn't be hard to figure out which.  Getting a backtrace is the way to move forward here.
You are adding a visit and that causes us to recalculate frecency for that page, this happens on the async thread though.

I touched this code today, so I could look why this warns, the problem may be minimal though, like caused by the subquery.
(In reply to comment #5)
> I touched this code today

That should be "I'm working on a patch that touches this code", but my patch is mostly removing some supporting code, not fixing any query or such.
(In reply to comment #5)

> I touched this code today, so I could look why this warns, the problem may
> be minimal though, like caused by the subquery.

I'll just say: we'd all love to see this go away :D  Thanks mak!
Component: Firefox Sync: Backend → Places
Product: Mozilla Services → Toolkit
QA Contact: sync-backend → places
Summary: Incoming history record processing causes SQL sort warning → Frecency update causes SQL sort warning
Assignee: nobody → mak77
Attached patch patch v1.0Splinter Review
I think we are going to see an increase in warnings, looks like current SQLite sometimes ignores existing indices, that's because if there are a few entries (like 2 or 3) doing sorting using a scan is faster than doing sorting using a b-tree, indeed in this case if I force it to use the index (through INDEXED BY moz_historyvisits_placedateindex) the warning goes away. All of this uses sqlite_stat1 (and, if present, sqlite_stat2) table, so ANALYZE is really important.
Based on this, I've added a /* do not warn */ since the indices exist but are ignored by the optimizer.

Apart this, the patch slightly improves the queries, and introduces 2 stataments caches for them, removing all the crazy stuff that was introduced to init those statements. In a separate bug we can remove GetStatementByID and GetStatement and directly use these caches for all statements.
Attachment #536268 - Flags: review?(sdwilsh)
Comment on attachment 536268 [details] [diff] [review]
patch v1.0

r=sdwilsh
Attachment #536268 - Flags: review?(sdwilsh) → review+
http://hg.mozilla.org/mozilla-central/rev/8b9ce2b1e438
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.