1452376 - Replace GetDescendantFolders with a recursive CTE subquery

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P2)

Description

[Marco Bonardo [:mak]]

off-hand, we should be able to just use a subquery for our needs.

Comment 1

[Richard Newman [:rnewman]]

I remember Kit hitting a situation where a recursive query was way slower than doing the work in JS; they might have useful input here.

Comment 2

[Marco Bonardo [:mak]]

Thanks for the heads up. In this case the hierarchy is very small and the requested info (just ids) is tiny, the querying overhead is likely always bigger.

Comment 3

[Marco Bonardo [:mak]]

it would pretty much be something like:

SELECT * FROM moz_bookmarks
WHERE parent IN(
	WITH RECURSIVE parents(id) AS (
	  VALUES(:parent)
	  UNION ALL
	  SELECT b2.id
	  FROM moz_bookmarks b2
	  JOIN parents p ON b2.parent = p.id
	  WHERE b2.type = 2
	)
	SELECT id FROM parents
)

using :parent = 2 in my case, returns 65 results, that means we currently run 65 queries, without the CTE.
Additionally, this code path is only taken by place: queries we DO NOT have in the product anymore after the tags rewrite (queries like "place:folder=something&some_other_options"), so it's an edge case.

Comment 4

[Lina Butler [:lina]]

(In reply to Richard Newman [:rnewman] from comment #1)
> I remember Kit hitting a situation where a recursive query was way slower
> than doing the work in JS; they might have useful input here.

Thanks for the heads-up, Richard! I think https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/toolkit/components/places/SyncedBookmarksMirror.jsm#1023-1027 is the case you're thinking of: a CTE with LEFT JOINs on a TEXT primary key in the base and recursive clauses. I'm not sure which one is to blame, though I did notice in bug 1436215, comment 1 that left joins and cascading deletes involving text keys are *very* slow.

Mak's query in comment 3 is simpler, and should be fast.

Comment 5

[Marco Bonardo [:mak]]

I guess I may just have a look at this, since we're removing cpp bookmarks APIs and this is one.

Comment 6

[Marco Bonardo [:mak]]

Attachment: Bug 1452376 - Replace GetDescendantFolders with a recursive subquery.

Review commit: https://reviewboard.mozilla.org/r/235106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235106/

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8966403 [details]
Bug 1452376 - Replace GetDescendantFolders with a recursive subquery.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235106/diff/1-2/

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8966403 [details]
Bug 1452376 - Replace GetDescendantFolders with a recursive subquery.

https://reviewboard.mozilla.org/r/235106/#review241000

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/827cc04dacce
Replace GetDescendantFolders with a recursive subquery. r=standard8

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/827cc04dacce