This starts to get hairy. We shouldn't have to worry about bookmarks (and it turns out some of the queries are really hard too) because we should always sync after adding a bookmark. At least I think...

That previous comment only holds for querying on moz_places. For moz_historyvisits, that is a horribly false statement.

This is just the conversion from moz_places to moz_places_view (and other complicated queries). This *does not* pass unit tests, and won't be able to because it requires the changes for moz_historyvisits. However, it'd be really awesome if folks could look over my query conversions and see if they see anything wrong.

Blocked on what I think is a sqlite bug: http://www.mail-archive.com/sqlite-users@sqlite.org/msg36069.html

(In reply to comment #3) > Blocked on what I think is a sqlite bug: > http://www.mail-archive.com/sqlite-users@sqlite.org/msg36069.html Actually, just me being stupid again. Better error messages may have been nice there, but oh well.

diff --git a/toolkit/components/places/src/nsAnnotationService.cpp b/toolkit/components/places/src/nsAnnotationService.cpp --- a/toolkit/components/places/src/nsAnnotationService.cpp +++ b/toolkit/components/places/src/nsAnnotationService.cpp @@ -155,18 +155,25 @@ nsAnnotationService::Init() "WHERE a.item_id = ?1"), getter_AddRefs(mDBGetItemAnnotationNames)); NS_ENSURE_SUCCESS(rv, rv); // mDBGetAnnotationFromURI rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( "SELECT a.id, a.place_id, ?2, a.mime_type, a.content, a.flags, " "a.expiration, a.type " - "FROM moz_places h JOIN moz_annos a ON h.id = a.place_id " - "WHERE h.url = ?1 AND a.anno_attribute_id = " + "FROM ( " + "SELECT * FROM moz_places_temp " + "WHERE url = ?1 " + "UNION ALL " + "SELECT * FROM moz_places " + "WHERE id NOT IN (SELECT id FROM moz_places_temp) " + "AND url = ?1 " + ") AS h JOIN moz_annos a ON h.id = a.place_id " + "WHERE a.anno_attribute_id = " well, since this query is going to use only (id, url) from moz_places you could do SELECT id, url instead of a SELECT * on both tables and then you could do a UNION ALL on really smaller tables. Maybe for the generic case in which you don't have to use an expression, you could write an helper like GetMozPlacesSqlString(list_of_fields) that could return the sql code needed to get correct fields, and use that when building queries, so they would still be readable end would be perf better. is doing a SELECT DISTINCT id on the unified table slower then using a id NOT IN (SELECT id)...? so could be faster doing something like(?): SELECT DISTINCT id, url FROM ( SELECT id, url FROM moz_places ... UNION ALL SELECT id, url FROM moz_places_tmp ... ) same for next queries, a perf check could be useful for example isPageVisited is selecting everything while it only needs id (or better it only needs to know if something exists @@ -722,16 +722,22 @@ nsNavHistory::InitDB(PRInt16 *aMadeChang } // Migrate historyvisits and bookmarks up to V7 if (DBSchemaVersion < 7) { rv = MigrateV7Up(mDBConn); NS_ENSURE_SUCCESS(rv, rv); } + // Migrate historyvisits up to V8 + if (DBSchemaVersion < 8) { + rv = MigrateV8Up(mDBConn); + NS_ENSURE_SUCCESS(rv, rv); + } + you should probably set PLACES_SCHEMA_VERSION to 8... i don't see such a change in the patch - "INSERT OR REPLACE INTO moz_places " + "INSERT OR REPLACE INTO moz_places_view " pay attention to the particular behaviour of insert or replace in sqlite, while in most other dbs if a row exists it will return the same id that already exists, IIRC in sqlite it removes the old row and creates a new one with a new id. probably will not create any problem but take this as a warning :) rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, " SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" ) ", f.url, null, null " - "FROM moz_places h " + "FROM moz_places_view h " "JOIN moz_historyvisits v ON h.id = v.place_id " "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id " "WHERE v.id = ?1"), getter_AddRefs(mDBVisitToURLResult)); NS_ENSURE_SUCCESS(rv, rv); is not really slower selecting from a view? i remember that it was, did they fixed it? If not this will be a perf hog in all places where you select or join against the view... "SELECT * FROM " "(SELECT id, visit_count, hidden, typed, frecency, url " - "FROM moz_places WHERE frecency < 0 " + "FROM ( " + "SELECT * FROM moz_places_temp " + "WHERE frecency < 0 " + "UNION ALL " + "SELECT * FROM moz_places " + "WHERE id NOT IN (SELECT id FROM moz_places_temp) " + "AND frecency < 0 " + ") " "ORDER BY frecency ASC LIMIT ROUND(?1 / 2)) " "UNION " "SELECT * FROM " "(SELECT id, visit_count, hidden, typed, frecency, url " - "FROM moz_places WHERE frecency < 0 " + "FROM ( " + "SELECT * FROM moz_places_temp " + "WHERE frecency < 0 " + "UNION ALL " + "SELECT * FROM moz_places " + "WHERE id NOT IN (SELECT id FROM moz_places_temp) " + "AND frecency < 0 " + ") " "ORDER BY RANDOM() LIMIT ROUND(?1 / 2))"), oh well this can probably be rewritten doing directly unions on singular tables and avoiding some nesting. like SELECT UNION ALL SELECT UION ALL SELECT... "SELECT rev_host FROM moz_places_temp " + "WHERE hidden <> 1 " + "AND rev_host <> '.' " + "AND visit_cout > 0 " + "UNION ALL " + "SELECT rev_host FROM moz_places " + "WHERE id NOT IN (SELECT id FROM moz_places_temp) " + "AND hidden <> 1 " + "AND rev_host <> '.' " + "AND visit_cout > 0 " TYPO: visit_cout ??? -#ifdef DEBUG_thunder +#ifdef DEBUG_sdwilsh printf("Constructed the query: %s\n", PromiseFlatCString(queryString).get()); #endif this should probably go before the final patch version /** * Trigger increments the visit count by one for each inserted visit that isn't * an invalid transition, embedded transition, or a download transition. + * XXX remove all references? This is no longer needed with view triggers */ #define CREATE_VISIT_COUNT_INSERT_TRIGGER NS_LITERAL_CSTRING( \ "CREATE TRIGGER moz_historyvisits_afterinsert_v1_trigger " \ "AFTER INSERT ON moz_historyvisits FOR EACH ROW " \ "WHEN NEW.visit_type NOT IN (0, 4, 7) " /* invalid, EMBED, DOWNLOAD */ \ "BEGIN " \ "UPDATE moz_places " \ "SET visit_count = visit_count + 1 " \ "WHERE moz_places.id = NEW.place_id; " \ "END" \ ) it is important that visit_count is real and effective, and never go out of synch... so since you're dropping the triggers above, if you can maintain the synch always these should go

(In reply to comment #5) > well, since this query is going to use only (id, url) from moz_places you could > do SELECT id, url instead of a SELECT * on both tables and then you could do a > UNION ALL on really smaller tables. Maybe for the generic case in which you > don't have to use an expression, you could write an helper like > GetMozPlacesSqlString(list_of_fields) that could return the sql code needed to > get correct fields, and use that when building queries, so they would still be > readable end would be perf better. I'm not sure it actually matters to sqlite. The temp table will be constructed once. I was trying to keep it all the same so I could tell what I had changed and what I hadn't yet wit grep. > is doing a SELECT DISTINCT id on the unified table slower then using a id NOT > IN (SELECT id)...? not 100% sure. I'm not really perf testing all these queries (and in fact, I think many of them it doesn't matter much). Once I get things working, I can attack perf more. > - "INSERT OR REPLACE INTO moz_places " > + "INSERT OR REPLACE INTO moz_places_view " > > pay attention to the particular behaviour of insert or replace in sqlite, while > in most other dbs if a row exists it will return the same id that already > exists, IIRC in sqlite it removes the old row and creates a new one with a new > id. probably will not create any problem but take this as a warning :) I don't think it matters at all for us - except when it comes to syncing. At that point, things are going to suck :( That is a problem for later on though! > is not really slower selecting from a view? i remember that it was, did they > fixed it? If not this will be a perf hog in all places where you select or join > against the view... It is slower, but like I said - there are places where I a) didn't think perf was terribly important or b) there was no way that I could see to not use the view without making the query essentially unreadable. More work needs to be done here. > oh well this can probably be rewritten doing directly unions on singular tables > and avoiding some nesting. like SELECT UNION ALL SELECT UION ALL SELECT... That's possible too. I've pretty much just been doing a mechanical rewrite wherever I see moz_places or moz_historyvisits > TYPO: visit_cout ??? fixed two instances of that

Improvements. Still not passing one of the autocomplete tests, and I'm not sure why (it presently gets as far as testing star restriction, which only gets two results when it expects eight). I've marked a few queries with XXX comments saying they need to be rewritten in a better optimized way. Suggestions welcome as I try to get the unit tests passing here. We are sadly running out of time.

This passes everything but a few tests left in the unit/ directory who's names are hard to figure out because my console is acting strange. I'm going to restart to see if it resolves things. This adds a bunch of queries that needs to be better optimized. They've been marked as such.

reboot fixed the console issue - yay! The following tests are failed: test_000_frecency.js test_399266.js test_425563.js test_expiration.js This is actually looking pretty good at this point finally.

So, I fixed a few more issues, but I'm still stuck on those four tests. My gut tells me that fixing expiration would fix at least one other test, but I'm kinda stuck. Help is desperately wanted here.

+ // Migrate historyvisits up to V8 + if (DBSchemaVersion < 8) { + rv = MigrateV8Up(mDBConn); + NS_ENSURE_SUCCESS(rv, rv); + } + // XXX Upgrades >V7 must add migration code here. fix comment "Upgrades >V8 must..." + // XXX investigate if this needs to be a view (does it happen before or after + // temp table creation?) + rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( + "UPDATE moz_places_view SET visit_count = " "(SELECT count(*) FROM moz_historyvisits " "WHERE place_id = moz_places.id " migrateV7Up will happen before migrateV8Up, so temp tables do not exists + nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( + "SELECT id " + "FROM ( " + "SELECT id FROM moz_historyvisits_temp " + "UNION ALL " + "SELECT id FROM moz_historyvisits " + "WHERE id NOT IN (SELECT id FROM moz_historyvisits_temp) " + ") " + "LIMIT 1"), + getter_AddRefs(dbSelectStatement)); LIMIT 1 is for perf reasons, we only need to know if the table contains something, so you should put limit 1 in the inner selects or you will end up selecting and unifying the full history table and then limiting so you could simply do + "SELECT id FROM moz_historyvisits_temp LIMIT 1 + "UNION ALL " + "SELECT id FROM moz_historyvisits LIMIT 1 and that is enough to know if we have some visit, overlap is not so important here - "UPDATE moz_places " + "UPDATE moz_places_view " "SET frecency = 0 WHERE id IN (" - "SELECT h.id FROM moz_places h " + "SELECT h.id FROM ( " + "SELECT * FROM moz_places_temp " + "WHERE frecency < 0 " + "OR SUBSTR(url, 0, 6) = 'place:' " + "UNION ALL " + "SELECT * FROM moz_places " + "WHERE id NOT IN (SELECT id FROM moz_places_temp) " + "AND (frecency < 0 " + "OR SUBSTR(url, 0, 6) = 'place:') " + ") AS h " "LEFT OUTER JOIN moz_bookmarks b ON h.id = b.fk " "WHERE frecency < 0 AND " // place is an unvisited child of a livemark feed you have already filtered on frecency < 0

(In reply to comment #9) > The following tests are failed: ... > test_expiration.js here's the failing test: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_expiration.js#567 this test is wrong. 1. the "expirable" visit is only 2 days old, which means it doesn't immediately meet any of the criteria set by the pref modifications done there. 2. the browser.history_expire_sites pref is being set to 2, which means that the trigger URI is what's triggering expiration of that expirable visit (as it's the 3rd site), not browser.history_expire_days, which is what the test is supposed to do. i'll provide a fix presently.

(In reply to comment #11) > + "SELECT id FROM moz_historyvisits_temp LIMIT 1 > + "UNION ALL " > + "SELECT id FROM moz_historyvisits LIMIT 1 sqlite doesn't let you specify limits with union all statements it turns out

Success! This, with patches in dependent bugs finally passes the unit tests. I had a bug with one of the triggers so frecency calculations were off for several things. Lame. Let's do the query optimization in a follow-up bug (that we'll need to actually land, but it makes sense to split this stuff up so we can work in parallel now).

(In reply to comment #12) > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_expiration.js#567 > > this test is wrong. > being handled in bug 450675.

Comment on attachment 333881 [details] [diff] [review] v1.0 first pass >@@ -1205,19 +1212,25 @@ nsAnnotationService::GetPagesWithAnnotat > > nsresult > nsAnnotationService::GetPagesWithAnnotationCOMArray( > const nsACString& aName, nsCOMArray<nsIURI>* aResults){ > // this probably isn't a common operation, so we don't have a precompiled > // statement. Perhaps this should change. > nsCOMPtr<mozIStorageStatement> statement; > nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >- "SELECT h.url FROM moz_anno_attributes n " >- "INNER JOIN moz_annos a ON n.id = a.anno_attribute_id " >- "INNER JOIN moz_places h ON a.place_id = h.id " >+ "SELECT h.url " >+ "FROM moz_anno_attributes n INNER JOIN moz_annos a " >+ "ON n.id = a.anno_attribute_id " >+ "INNER JOIN ( " >+ "SELECT * FROM moz_places_temp " >+ "UNION ALL " >+ "SELECT * FROM moz_places " >+ "WHERE id NOT IN (SELECT id FROM moz_places_temp) " >+ ") h ON a.place_id = h.id " > "WHERE n.name = ?1"), > getter_AddRefs(statement)); > NS_ENSURE_SUCCESS(rv, rv); nit: keep the INNER JOIN on it's own line for readability > // mDBGetChildren: select all children of a given folder, sorted by position > // This is a LEFT OUTER JOIN with moz_places since folders does not have > // a reference into that table. >+ // XXX optimize? there are variants such as "this is inefficient", "optimize this", "optimize me" throughout the patch. please standardize them using something easily findable, such as bug #. >@@ -722,16 +724,22 @@ nsNavHistory::InitDB(PRInt16 *aMadeChang > } > > // Migrate historyvisits and bookmarks up to V7 > if (DBSchemaVersion < 7) { > rv = MigrateV7Up(mDBConn); > NS_ENSURE_SUCCESS(rv, rv); > } > >+ // Migrate historyvisits up to V8 >+ if (DBSchemaVersion < 8) { >+ rv = MigrateV8Up(mDBConn); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ > // XXX Upgrades >V7 must add migration code here. fix comment > // mDBGetURLPageInfo > rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( > "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count " >- "FROM moz_places h " >- "WHERE h.url = ?1"), >+ "FROM ( " >+ "SELECT * FROM moz_places_temp " >+ "WHERE url = ?1 " >+ "UNION ALL " >+ "SELECT * FROM moz_places " >+ "WHERE id NOT IN (SELECT id FROM moz_places_temp) " >+ "AND url = ?1 " >+ ") AS h"), nit: could remove the table aliasing to save a few nanoseconds >+ // XXX this can be better optimized > rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( > "SELECT IFNULL(r.visit_date, v.visit_date) date, IFNULL(r.visit_type, v.visit_type) " >- "FROM moz_historyvisits v " >- "LEFT OUTER JOIN moz_historyvisits r " >- "ON r.id = v.from_visit AND v.visit_type IN ") + >+ "FROM ( " >+ "SELECT * FROM moz_historyvisits_temp " >+ "WHERE place_id = ?1 " >+ "UNION ALL " >+ "SELECT * FROM moz_historyvisits " >+ "WHERE id NOT IN (SELECT id FROM moz_historyvisits_temp) " >+ "AND place_id = ?1 " >+ ") AS v " >+ "LEFT OUTER JOIN moz_historyvisits_view r " >+ "ON r.id = v.from_visit " >+ "AND v.visit_type IN ") + is there a reason why you UNION'd the first bit, but used the view on the join? >@@ -1430,18 +1550,19 @@ nsNavHistory::MigrateV7Up(mozIStorageCon > NS_ENSURE_SUCCESS(rv, rv); > > // We need to create two triggers on moz_historyvists to maintain the > // accuracy of moz_places.visit_count. For this to work, we must ensure that > // all moz_places.visit_count values are correct. > // See bug 416313 for details. > if (!triggerExists) { > // First, we do a one-time reset of all the moz_places.visit_count values. >- rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >- "UPDATE moz_places SET visit_count = " >+ // temp table creation?) >+ rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >+ "UPDATE moz_places_view SET visit_count = " > "(SELECT count(*) FROM moz_historyvisits " > "WHERE place_id = moz_places.id " > "AND visit_type NOT IN (0, 4, 7))") /* invalid, EMBED, DOWNLOAD */ i don't understand the comment line you added. > nsresult > nsNavHistory::FixInvalidFrecenciesForExcludedPlaces() > { > // for every moz_place that has an invalid frecency (< 0) and > // is an unvisited child of a livemark feed, or begins with "place:", > // set frecency to 0 so that it is excluded from url bar autocomplete. > nsCOMPtr<mozIStorageStatement> dbUpdateStatement; > nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >- "UPDATE moz_places " >+ "UPDATE moz_places_view " > "SET frecency = 0 WHERE id IN (" >- "SELECT h.id FROM moz_places h " >+ "SELECT h.id FROM ( " >+ "SELECT * FROM moz_places_temp " >+ "WHERE frecency < 0 " >+ "OR SUBSTR(url, 0, 6) = 'place:' " >+ "UNION ALL " >+ "SELECT * FROM moz_places " >+ "WHERE id NOT IN (SELECT id FROM moz_places_temp) " >+ "AND (frecency < 0 " >+ "OR SUBSTR(url, 0, 6) = 'place:') " >+ ") AS h " > "LEFT OUTER JOIN moz_bookmarks b ON h.id = b.fk " >- "WHERE frecency < 0 AND " >+ "WHERE " > // place is an unvisited child of a livemark feed > "(b.parent IN (" > "SELECT annos.item_id FROM moz_anno_attributes attrs " > "JOIN moz_items_annos annos ON attrs.id = annos.anno_attribute_id " > "WHERE attrs.name = ?1) " > "AND visit_count = 0) " > "OR SUBSTR(h.url,0,6) = 'place:')"), > getter_AddRefs(dbUpdateStatement)); the SUBSTR in the WHERE clause can be removed, as it's in the FROM subselect now, right? > >-#ifdef DEBUG_FRECENCY >+#ifdef DEBUG_sdwilsh :/ > // XXX bug 412736 > // in the case of a frecency tie, break it with h.typed and h.visit_count > // which is better than nothing. but this is slow, so not doing it yet. > nsCString sqlTail = NS_LITERAL_CSTRING( > "ORDER BY h.frecency DESC LIMIT ?2 OFFSET ?3"); >+ // XXX the above is likely slow since we can't sort on an index anymore > why? and also please mark this for optimization, such that it's findable. >+ placeIds + NS_LITERAL_CSTRING( >+ ") " >+ ") " >+ ")")); this hurts. > /** > * Trigger increments the visit count by one for each inserted visit that isn't > * an invalid transition, embedded transition, or a download transition. >+ * XXX remove all references? This is no longer needed with view triggers > */ yes, please remove all references to the now non-existent triggers

before this lands, we must be able to measure performance of various actions that flex this code. a low-bar approach i've been thinking about is to write chrome mochitests that contain perf scores in the test comments. for example, we could write a mochitest that times the opening of the history menu, and the test comment could be "ui-perf|places|open history menu|20ms" or something like that. eventually we could build a datastore and UI, making it generally useful, but the log data alone provides a simple solution for our immediate needs.

i'll attach a results table (with patch) to bug 450705 to check the perf changes between original version, partitioned version, optimized partitioned version (for each query). still that complete test will require at least one day since it's a manual work. A mochitest with some printout will not help query rewriting, still would be useful to see the perf impact of this change on the ui. I've added to my local repository all latest version patches and i can't get it running, it compiles fine but i cannot execute tests nor browser, so i'm looking after to fix the build bustage (for now i've found that temporary tables patch does not create temp tables but classic on-disk ones).

That's weird. I'm not sure why it wouldn't run for you when it ran for deitrich and I :(

don't worry, it was a build bustage on my side, fixed and i'm proceding with perf tests... there is some query really *problematic*, so i'm proceding with caution

(In reply to comment #16) > nit: could remove the table aliasing to save a few nanoseconds I'll leave that for Marco's bug > is there a reason why you UNION'd the first bit, but used the view on the join? I could optimize with the index on the first bit, but not on the second bit > >+ // XXX the above is likely slow since we can't sort on an index anymore > why? and also please mark this for optimization, such that it's findable. Because the index is now in two different tables (moz_places and moz_plaeces_temp). > >+ placeIds + NS_LITERAL_CSTRING( > >+ ") " > >+ ") " > >+ ")")); > this hurts. While it may hurt a bit, it goes a long way when you are debugging queries. I had a really difficult time trying to figure out what braces grouped what half the time.

Comment on attachment 334314 [details] [diff] [review] v1.1 r=me, thanks.

notice that imho we don't want this without optimized queries. Why? I've seen a query moving from 12ms to 23s, and that's not good when that query is used to populate the bookmarks menu, so if this goes pushed we should push optimized queries immediately after.

(In reply to comment #23) > notice that imho we don't want this without optimized queries. Why? I've seen a > query moving from 12ms to 23s, and that's not good when that query is used to > populate the bookmarks menu, so if this goes pushed we should push optimized > queries immediately after. > undoubtedly. this is one in a series of patches for this project. these things still need to be done before we push: - run a try-server build, to see Ts/Tp effect (looky: http://blog.mozilla.com/jorendorff/2008/08/18/push-to-try/) - build some basic performance tests w/ mochitest to measure common UI actions that flex these code paths, to get a baseline and measure the delta post-partitioning - get test builds in the hands of suffering linux/ext3 users to determine if this actually resolves the issue for them - and if possible, get the portable firefox folks to generate a build with these patches and confirm that this approach improves performance for their product

analisys continues... mDBGetRedirectDestinations is filtering differently from before, you are filtering before a LEFT JOIN while previous it was filtering after. For this reason null could be returned.

How is that the case? If was filtering what rows were being displayed based on columns in that table, the left join wouldn't change. If it were a right join I'd agree, but the left join shouldn't matter.

"SELECT dest_v.place_id " - "FROM moz_historyvisits source_v " - "LEFT JOIN moz_historyvisits dest_v ON dest_v.from_visit = source_v.id " - "WHERE source_v.place_id = ?1 " - "AND source_v.visit_date >= ?2 " - "AND (dest_v.visit_type = 5 OR dest_v.visit_type = 6) " "GROUP BY dest_v.place_id") original query does a left join and then filter out on visit_type (why not doing directly a join then, since we need a result from the joined table??), if the expression does not find anything you get anything. "SELECT dest_v.place_id " + "FROM ( " + "SELECT * FROM moz_historyvisits_temp " + "WHERE place_id = ?1 " + "AND visit_date >= ?2 " + "UNION ALL " + "SELECT * FROM moz_historyvisits " + "WHERE id NOT IN (SELECT id FROM moz_historyvisits_temp) " + "AND place_id = ?1 " + "AND visit_date >= ?2 " + ") AS source_v " + "LEFT JOIN ( " + "SELECT * FROM moz_historyvisits_temp " + "WHERE visit_type IN (5, 6) " + "UNION ALL " + "SELECT * FROM moz_historyvisits " + "WHERE id NOT IN (SELECT id FROM moz_historyvisits_temp) " + "AND visit_type IN (5, 6) " + ") AS dest_v " + "ON dest_v.from_visit = source_v.id " "GROUP BY dest_v.place_id") new query filter out the second table and then makes a left join, if the first table has a result but the second does not, you get a null row as resultset.

also, the reset frecency query in EraseVisits appear wrong, double check parenthesis - "v.id NOT IN (") + deletedVisitIds + - NS_LITERAL_CSTRING(")) AND " notice double closing before AND + "AND v.id NOT IN ( ") + deletedVisitIds + NS_LITERAL_CSTRING( + ") AND h.id IN (") + no more, so probably h.id filter is applied inside the NOT EXISTS while it was outside before

also the query in FixInvalidFrecenciesForExcludedPlaces is returning wrong resultset, the check on feeds was an OR, while now is a condition for all rows. notice i'll fix queries in bug 450705 but i think is still worth marking errors here.

Comment on attachment 334314 [details] [diff] [review] v1.1 r=me until this is checked in with patch from bug 450705 (see comments above for the reasons)

Comment on attachment 334314 [details] [diff] [review] v1.1 This patch has been merged in patch from bug 450705. For maintainance having only 1 patch to unbitrot is better and safer.