Make left pane queries virtual

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: lina, Assigned: standard8)

Tracking

(Blocks 6 bugs, {perf})

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [qf:p3])

Attachments

(2 attachments)

These queries are currently created by http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/components/places/PlacesUIUtils.jsm#1138-1156, and stored as children of the Places root.

In bug 1258127, comment 190, Mak suggests making them virtual, or hardcoding the list of queries:

> The left pane folder is something that could probably be converted to a
> "virtual" folder. By this I mean we could have something like
> RESULTS_AS_TAG_QUERY, but it being RESULTS_AS_LEFTPANE_FOLDER that returns a
> fake result. Or we could hardcode the Library left pane contents, it's a
> long time we want to move the library to an in-content UI, and the current
> tree is not the best visual for that. That may allow us to have no real left
> pane folder.
Priority: -- → P3
Blocks: 1161091
Priority: P3 → P2
Whiteboard: [qf]
Duplicate of this bug: 1161091
Reporter

Updated

2 years ago
See Also: → PlacesInTabLibrary
Marco is this related to the code that powers the primary bookmarking UI? If it is than it should be a qf:p1. Please let us know. Thanks.
Flags: needinfo?(mak77)
Whiteboard: [qf] → [qf:p1]
This is needed to be able to remove the old synchronous bookmarking API, as most of the other blockers of bug 519514. The code is used by the Library left pane.
As a bonus, it would also go towards allowing us to simplify some of the Sync code, making synchronization cheaper.
Flags: needinfo?(mak77)

Comment 4

2 years ago
If this improves the performance of the Library window which isn't part of our primary UI we should probably bump down the priority to P3...
Whiteboard: [qf:p1] → [qf:p3]
Keywords: perf
Blocks: 1410877
Blocks: 1083465
Assignee

Comment 5

2 years ago
I've started looking at this.
Assignee: nobody → standard8
Status: NEW → ASSIGNED
I suggest to start by replacing allBookmarksFolderId, that is a subquery and should be simpler.
Then the whole left pane folder will return this new query among the others.
Assignee

Updated

2 years ago
Depends on: 1423896
Blocks: 824502
Assignee

Updated

Last year
Priority: P2 → P1
Assignee

Comment 8

Last year
I've just attached the current WIP. The basic view works in the left-pane, however the test failures show there's various selection issues that I need to resolve yet. I suspect there's a few other bits to fix up yet, including the XXX comments I've left in there.
Blocks: 1441695
Blocks: 1441697
Blocks: 1441698
No longer blocks: 1083465
Blocks: 1437516
Comment hidden (mozreview-request)
Assignee

Comment 10

Last year
I think the patch is now functionally complete for the left pane. The only thing left to do is to add a migration routine for the database to remove any non-default root items.

I'm letting it run on try overnight, and I'll do some more manual testing tomorrow, however, any comments welcome in the meantime.
Assignee

Comment 11

Last year
Having discussed with Marco on IRC, we've been through the options of using database migration, UI migration & idle places maintenance.

The maintenance isn't really an option because this doesn't guarantee to get us to the position where "everything in the db can be synced". The UI migration isn't viable as it doesn't re-migrate if the user downgrades and then upgrades.

This leaves us with database migration, which is possible but more complex than the other two options, as we can't use the already written bookmark APIs already available to us.

In Marco's words, this is what we need to do:

temp trigger to update foreign_count, recursive depth-first CTE query to delete all the bookmarks, remove orphan places and annos... 


Kit, is it ok to delete these without Sync noticing, or do we need to do more work?
Flags: needinfo?(kit)
(In reply to Mark Banner (:standard8) from comment #11)
> Kit, is it ok to delete these without Sync noticing, or do we need to do
> more work?

This sounds like a good plan! I think the temp trigger should also insert the removed GUIDs into `moz_bookmarks_deleted`, so that we upload tombstones for them.

Since bug 1274496, Sync ignores items that aren't under the four user content roots. However, for older accounts, some or all of the left pane items might have been accidentally uploaded and synced to other devices, causing issues like bug 1436897. Other devices will ignore tombstones for items that they don't know about, so this is OK to do unconditionally.
Flags: needinfo?(kit)
Comment hidden (mozreview-request)
Assignee

Comment 15

Last year
Status update:

I think the main patch here is ready to start the review cycle. I'm still working on the migration patch (only test written so far), but I think it is separate enough that we can start reviews progressing on the main changes.
Assignee

Comment 16

Last year
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

I've created a test for the migration that I think should cover everything we need to check post-migration. If you could take a look that'll be good.

In the meantime, I'll start thinking about writing the actual migration routine.
Attachment #8956125 - Flags: feedback?(mak77)

Comment 17

Last year
mozreview-review
Comment on attachment 8954107 [details]
Bug 1310295 - Make left pane queries virtual in the Places Library window.

https://reviewboard.mozilla.org/r/223252/#review231624

a discussed over IRC, we need a blocker bug preventing "insert" and "update" adding stuff into the root

::: toolkit/components/places/nsNavHistoryResult.cpp:2122
(Diff revision 3)
>    nsNavHistoryResult* result = GetResult();
>    NS_ENSURE_STATE(result);
>  
> -  if (mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY ||
> -      mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) {
> +  if ((mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY ||
> +       mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) &&
> +      mLiveUpdate != QUERYUPDATE_NONE) {

what about instead we bailout early with 

if (mLiveUpdate == QUERYUPDATE_NONE) {
  mContentsValid = true;
  return NS_OK;
}
Attachment #8954107 - Flags: review?(mak77) → review+
Assignee

Updated

Last year
Depends on: 1443835
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

looks good so far
Attachment #8956125 - Flags: feedback?(mak77) → feedback+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

Last year
Note: some of the test removals from the first patch have now moved to bug 1443835. I'm intending to land these at the nearly same time and it made sense to do that, rather than fix up the tests.
Reporter

Comment 22

Last year
mozreview-review
Comment on attachment 8954107 [details]
Bug 1310295 - Make left pane queries virtual in the Places Library window.

https://reviewboard.mozilla.org/r/223252/#review232280

Awesome!
Attachment #8954107 - Flags: review?(kit) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 25

Last year
mozreview-review
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

https://reviewboard.mozilla.org/r/225056/#review232618

Unsolicited drive-by SQL comments. :-)

::: toolkit/components/places/Database.cpp:2006
(Diff revision 3)
> +
> +  // This trigger listens for moz_places deletes, and updates moz_annos and
> +  // moz_keywords accordingly.
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "CREATE TEMP TRIGGER moz_migrate_annos_trigger "
> +    "AFTER DELETE ON moz_places FOR EACH ROW "

Triggers cascade, so you could also make this an `AFTER UPDATE` trigger `WHEN NEW.foreign_count = 0 AND NEW.visit_count = 0`, and take out the `DELETE FROM moz_places` above.

::: toolkit/components/places/Database.cpp:2032
(Diff revision 3)
> +    "    WHERE id = OLD.place_id; "
> +    "END "
> +  ));
> +  if (NS_FAILED(rv)) return rv;
> +
> +  // First of all, find the non-builtin roots.

If you wanted to, I think you could do the deletions in a single statement, and mop up non-folder children of the Places root with something like:

```
WITH RECURSIVE
itemsToRemove(id, guid) AS (
  SELECT b.id, b.guid FROM moz_bookmarks b
  WHERE b.guid = 'root________'
  UNION ALL
  SELECT b.id, b.guid FROM moz_bookmarks b
  JOIN itemsToRemove d ON d.id = b.parent
  WHERE b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______')
)
DELETE FROM itemsToRemove
WHERE guid <> 'root________'
```

If you intentionally broke out `DeleteBookmarkFolderContents`, or Mak thinks differently, please ignore me. :-)

::: toolkit/components/places/Database.cpp:2093
(Diff revision 3)
> +  ));
> +  if (NS_FAILED(rv)) return rv;
> +
> +  // Cleanup any orphan annotation attributes.
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "DELETE FROM moz_anno_attributes WHERE id IN ( "

Nit: I think you can remove the subselect on `moz_anno_attributes` and do `DELETE FROM moz_anno_attributes WHERE NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.anno_attribute_id = id) ...`.
Reporter

Comment 26

Last year
mozreview-review-reply
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

https://reviewboard.mozilla.org/r/225056/#review232618

> Nit: I think you can remove the subselect on `moz_anno_attributes` and do `DELETE FROM moz_anno_attributes WHERE NOT EXISTS (SELECT a.id FROM moz_annos a WHERE a.anno_attribute_id = id) ...`.

Sorry, I'm totally wrong. >.<

Comment 27

Last year
mozreview-review
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

https://reviewboard.mozilla.org/r/225056/#review232770

::: toolkit/components/places/Database.cpp:1985
(Diff revision 3)
> +    "CREATE TEMP TRIGGER moz_migrate_bookmarks_trigger "
> +    "AFTER DELETE ON moz_bookmarks FOR EACH ROW "
> +    "BEGIN "
> +    // Insert tombstones.
> +    "  INSERT INTO moz_bookmarks_deleted (guid, dateRemoved) "
> +    "    VALUES (OLD.guid, %lld); "

nit: fwiw, Sqlite has time functions, for example yo ucan get a timestamp with
SELECT strftime('%s', 'now', 'localtime', 'utc') * 1000

::: toolkit/components/places/Database.cpp:2032
(Diff revision 3)
> +    "    WHERE id = OLD.place_id; "
> +    "END "
> +  ));
> +  if (NS_FAILED(rv)) return rv;
> +
> +  // First of all, find the non-builtin roots.

Kit suggestion to do the delete one should could be more performante.

I'd also like if we'd first run a select to check if there's anything to remove, and in case there isn't bailout even before creating the triggers.
I want to limit work for profiles that don't have anything to delete (the majority)

So I'd say to run a select to see if there's any extraneous root, if there isn't any just bailout.
Otherwise, add the triggers and do the one shot removal.

::: toolkit/components/places/Database.cpp:2058
(Diff revision 3)
> +
> +    int32_t type;
> +    rv = stmt->GetInt32(2, &type);
> +    if (NS_FAILED(rv)) return rv;
> +
> +    printf("id: %d, guid: %s, type: %d\n", id, guid.get(), type);

debug leftover

::: toolkit/components/places/Database.cpp:2077
(Diff revision 3)
> +    "  foreign_count = (SELECT COUNT(*) FROM moz_keywords WHERE place_id = p.id) "
> +    ")"
> +  ));
> +  if (NS_FAILED(rv)) return rv;
> +
> +  // Now remove the temp trigger.

nit: triggers (plural)

::: toolkit/components/places/Database.cpp:2099
(Diff revision 3)
> +    "  SELECT id from moz_anno_attributes n "
> +    "  WHERE NOT EXISTS "
> +    "    (SELECT id FROM moz_annos WHERE anno_attribute_id = n.id LIMIT 1) "
> +    "  AND NOT EXISTS "
> +    "    (SELECT id FROM moz_items_annos WHERE anno_attribute_id = n.id LIMIT 1) "
> +    ")"

Probably you could use EXCEPT in a more efficient way. (also uppercase FROM, per Places SQL coding style).
Please double check it does the right thing, usually selects group from left to right:

SELECT id FROM moz_anno_attributes
EXCEPT
SELECT DISTINCT anno_attribute_id FROM moz_annos
EXCEPT
SELECT DISTINCT anno_attribute_id FROM moz_items_annos

::: toolkit/components/places/tests/migration/test_current_from_v43.js:45
(Diff revision 3)
> +  Assert.equal(PlacesUtils.history.databaseStatus,
> +               PlacesUtils.history.DATABASE_STATUS_UPGRADED);
> +
> +  let db = await PlacesUtils.promiseDBConnection();
> +  Assert.equal((await db.getSchemaVersion()), CURRENT_SCHEMA_VERSION);
> +});

MAybe we should open a connection to v43 and sanity check the initial status? do we trust the tombstone check and review process enough to prevent people messing with the v43 file?
Attachment #8956125 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

Last year
mozreview-review
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

https://reviewboard.mozilla.org/r/225056/#review233414

::: toolkit/components/places/Database.cpp:1967
(Diff revision 5)
> +      "SELECT b.id, b.guid FROM moz_bookmarks b "
> +      "JOIN itemsToRemove d ON d.id = b.parent "
> +      "WHERE b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') "
> +    ") "
> +    "SELECT * FROM itemsToRemove "
> +      "WHERE guid <> 'root________' LIMIT 1 "

We don't need to be recursive here, you could just:
SELECT id
FROM moz_bookmarks b
JOIN moz_bookmarks p ON b.parent = p.id
WHERE p.guid = "root________"
  AND b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______')

You should also put this stmt into a scope, so it's finalized immediately on scope exist.

Anyway, as you told me on IRC the likelihood that there's nothing to remove is very low, since at the minimum there is the left pane folder, so I was probably overzealous about this early bailout. If you want to remove it completely I won't complain.

::: toolkit/components/places/Database.cpp:1987
(Diff revision 5)
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "CREATE TEMP TRIGGER moz_migrate_bookmarks_trigger "
> +    "AFTER DELETE ON moz_bookmarks FOR EACH ROW "
> +    "BEGIN "
> +      // Insert tombstones.
> +      "INSERT INTO moz_bookmarks_deleted (guid, dateRemoved) "

Just in case, I'd use INSERT OR IGNORE here, since guid is a primary key in moz_bookmarks_deleted

::: toolkit/components/places/Database.cpp:1988
(Diff revision 5)
> +    "CREATE TEMP TRIGGER moz_migrate_bookmarks_trigger "
> +    "AFTER DELETE ON moz_bookmarks FOR EACH ROW "
> +    "BEGIN "
> +      // Insert tombstones.
> +      "INSERT INTO moz_bookmarks_deleted (guid, dateRemoved) "
> +        "VALUES (OLD.guid, (SELECT strftime('%s', 'now', 'localtime', 'utc') * 1000)); "

you can avoid the sub select and just insert strftime() as a value, for example

INSERT INTO test VALUES (NULL, strftime('%s', 'now', 'localtime', 'utc') * 1000)

::: toolkit/components/places/Database.cpp:2008
(Diff revision 5)
> +    "CREATE TEMP TRIGGER moz_migrate_annos_trigger "
> +    "AFTER UPDATE ON moz_places FOR EACH ROW "
> +    // Only remove from moz_places if we don't have any remaining keywords pointing to
> +    // this place, and it hasn't been visited. Note: orphan keywords are tidied up below.
> +    "WHEN NEW.foreign_count = (SELECT COUNT(*) FROM moz_keywords WHERE place_id = NEW.id) AND "
> +      "NEW.visit_count = 0 "

let's put the visit_count check first... ideally the optimizer should already do that, but hinting it is better

::: toolkit/components/places/Database.cpp:2050
(Diff revision 5)
> +      "SELECT b.id, b.guid FROM moz_bookmarks b "
> +      "JOIN itemsToRemove d ON d.id = b.parent "
> +      "WHERE b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') "
> +    ") "
> +    "DELETE FROM moz_bookmarks "
> +      "WHERE guid <> 'root________' AND id IN (SELECT id FROM itemsToRemove) "

I think you can rewrite the CTE as

WITH RECURSIVE itemsToRemove(id, guid) AS ( 
   SELECT b.id, b.guid FROM moz_bookmarks b
   JOIN moz_bookmarks p ON b.parent = p.id
   WHERE p.guid = 'root________' 
     AND b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______') 
   UNION ALL 
   SELECT b.id, b.guid FROM moz_bookmarks b 
   JOIN itemsToRemove d ON d.id = b.parent 
)

And then avoid the guid <> root condition outside, the CTE will already select all the unexpected children of root, and their children

::: toolkit/components/places/Database.cpp:2064
(Diff revision 5)
> +  // so that we can make use of the keyword trigger to update the counts in
> +  // moz_places.
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "DELETE FROM moz_keywords WHERE place_id IN ( "
> +      "SELECT id FROM moz_places p WHERE "
> +        "foreign_count != 0 AND "

please use <> instead of !=, the latter is not a SQL standard

::: toolkit/components/places/Database.cpp:2066
(Diff revision 5)
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "DELETE FROM moz_keywords WHERE place_id IN ( "
> +      "SELECT id FROM moz_places p WHERE "
> +        "foreign_count != 0 AND "
> +        "foreign_count = (SELECT COUNT(*) FROM moz_keywords WHERE place_id = p.id) "
> +    ")"

For the internal select, I think this may be a bit faster, because there is no index on foreign_count, and your query is selecting based on its value, that ends up doing a SCAN TABLE on moz_places

SELECT h.id FROM moz_keywords k
JOIN moz_places h ON h.id = k.place_id
GROUP BY place_id HAVING h.foreign_count = count(*)

::: toolkit/components/places/Database.cpp:2091
(Diff revision 5)
> +    "DELETE FROM moz_anno_attributes WHERE id IN ( "
> +      "SELECT id FROM moz_anno_attributes n "
> +      "EXCEPT "
> +        "SELECT DISTINCT anno_attribute_id FROM moz_annos "
> +      "EXCEPT "
> +        "SELECT DISTINCT anno_attribute_id FROM moz_items_annos "

nit: usually we don't indent for UNION,INTERSECT,EXCEPT. So just:
SELECT
EXCEPT
SELECT
EXCEPT
SELECT
Attachment #8956125 - Flags: review?(mak77) → review+
Assignee

Comment 33

Last year
mozreview-review-reply
Comment on attachment 8956125 [details]
Bug 1310295 - Provide a Places database migration routine to remove non-built-in roots.

https://reviewboard.mozilla.org/r/225056/#review233414

> We don't need to be recursive here, you could just:
> SELECT id
> FROM moz_bookmarks b
> JOIN moz_bookmarks p ON b.parent = p.id
> WHERE p.guid = "root________"
>   AND b.guid NOT IN ('menu________', 'toolbar_____', 'tags________', 'unfiled_____', 'mobile______')
> 
> You should also put this stmt into a scope, so it's finalized immediately on scope exist.
> 
> Anyway, as you told me on IRC the likelihood that there's nothing to remove is very low, since at the minimum there is the left pane folder, so I was probably overzealous about this early bailout. If you want to remove it completely I won't complain.

I just checked with creating a blank profile in FF 58.0.2 and creating one bookmark - the left-pane items do get created. Hence I'm going to drop this check.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

Last year
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1af75c617ea
Make left pane queries virtual in the Places Library window. r=kitcambridge,mak
https://hg.mozilla.org/integration/autoland/rev/a277657bfffa
Provide a Places database migration routine to remove non-built-in roots. r=mak
Assignee

Comment 38

Last year
We're working on the mochitest failures. The TV failures are tier-2, and appear to be already existing (xref bug 1436175).
Flags: needinfo?(standard8)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 41

Last year
Try results with the fixed patch look good, so pushing again.

Comment 42

Last year
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb12946f24c7
Make left pane queries virtual in the Places Library window. r=kitcambridge,mak
https://hg.mozilla.org/integration/autoland/rev/4f96e58f2ece
Provide a Places database migration routine to remove non-built-in roots. r=mak
Blocks: 1310299
https://hg.mozilla.org/mozilla-central/rev/eb12946f24c7
https://hg.mozilla.org/mozilla-central/rev/4f96e58f2ece
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Updated

Last year
Depends on: 1446672
Depends on: 1472127
You need to log in before you can comment on or make changes to this bug.