The "My issues" view (e.g. http://drupal.org/project/issues/user) results in a *really* expensive query. nnewton and I (and previously, merlinofchaos) have been discussing it this week, and I've been thinking about it a lot. I'm finally writing this up as an issue. Once the redesign launches with a block of this view in the default user dashboard, we might really melt the d.o DB server.
Here's the query of doom:
SELECT node.nid AS nid,
node_project_issues_1.title AS node_project_issues_1_title,
node_project_issues_1.nid AS node_project_issues_1_nid,
node_project_issues_1__project_projects.uri AS node_project_issues_1__project_projects_uri,
node.title AS node_title,
history_user.timestamp AS history_user_timestamp,
node.created AS node_created,
node.changed AS node_changed,
project_issues.sid AS project_issues_sid,
project_issues.priority AS project_issues_priority,
project_issues.category AS project_issues_category,
node_project_issues__project_release_nodes.version AS node_project_issues__project_release_nodes_version,
node_project_issues.nid AS node_project_issues_nid,
node_comment_statistics.comment_count AS node_comment_statistics_comment_count,
node.type AS node_type,
node_comment_statistics.last_comment_timestamp AS node_comment_statistics_last_comment_timestamp,
users_project_issues.name AS users_project_issues_name,
users_project_issues.uid AS users_project_issues_uid
FROM node node
INNER JOIN project_issues project_issues ON node.nid = project_issues.nid
INNER JOIN users users_project_issues ON project_issues.assigned = users_project_issues.uid
LEFT JOIN node node_project_issues ON project_issues.rid = node_project_issues.nid
INNER JOIN node node_project_issues_1 ON project_issues.pid = node_project_issues_1.nid
INNER JOIN project_projects node_project_issues_1__project_projects ON node_project_issues_1.nid = node_project_issues_1__project_projects.nid
LEFT JOIN history history_user ON node.nid = history_user.nid AND history_user.uid = 544300
LEFT JOIN project_release_nodes node_project_issues__project_release_nodes ON node_project_issues.nid = node_project_issues__project_release_nodes.nid
INNER JOIN node_comment_statistics node_comment_statistics ON node.nid = node_comment_statistics.nid
WHERE (node.status <> 0 OR (node.uid = 544300 AND 544300 <> 0) OR 0 = 1) AND (project_issues.sid in ('1', '13', '8', '14', '15', '2', '4', '16')) AND (node.uid = 544300 OR ((SELECT COUNT(*) FROM comments c WHERE c.uid = 544300 AND c.nid = node.nid) > 0))
ORDER BY node_comment_statistics_last_comment_timestamp DESC
LIMIT 0, 50;
There are a couple a major problems here. In fact, most of the issue views have most of these problems.
- ORDER BY {node_comment_statistics} screws us with WHERE and ORDER BY on different tables. Makes using an index impossible.
- WHERE really sucks. {node}.status, lots of stuff from {project_issues}, and then the crazy subquery for finding comments you participated in. So, we're doing WHERE from at least 3 different tables, and this nasty dependent subquery is pain.
There are a few possible solutions here.
A) Add a ton of caching, for example, via http://drupal.org/project/views_content_cache (which we might want anyway over at #921124: Download And Extend - Module list View). However, we can obviously only cache this block per user, so we'd be looking at half million cache entries. And, this is the kind of block that people are going to expect to have real-time values in, so the lifetime would have to be so short, this approach probably wouldn't really help anyway.
B) Change the definition of "My issues" to be something easier to query for. E.g. only issues assigned to you. Or use flag. However, this doesn't really solve much on its own, since we'd still have at least 2 different tables in WHERE, and ORDER BY would still screw us. Plus, users would *hate* this if we changed the behavior to just use assigned. That's not a option. So, really, the only viable solution here would be flag. But, even a "minimal program" is still going to require a lot more development time on my part (e.g. to get the bare minimum of email support working based on flag), so it's a bit scary to try to fix all that before the redesign launch -- it's introducing a lot more moving pieces that could potentially break something.
C) Try to denormalize everything we can think of into Solr and use views_solr. However, I don't think this is really going to help with the equivalent of WHERE, and it's still going to require a fair bit of configuration work to get the extra data injected into the solr doc. Plus, it's unclear the stability and status of views_solr. Seems like a potentially huge rathole that might not even help that much in the end.
D) Use materialized views. Unfortunately, the views support to MV is still mostly vaporware, and it might involve me having to maintain two whole sets of issue views -- one set that talks to native tables, and another set that can talk to MV tables.
E) Manually denormalize some key things into {project_issues}. In particular, these 3 values:
{node}.status
{node_comment_statistics}.last_comment_timestamp
{node_comment_statistics}.comment_count
This would require:
E.1) hook_nodeapi() to notice whenever a node is being updated to keep {project_issues}.status accurate.
E.2) hook_comment() to notice whenever a comment is posted to an issue and update {project_issues}.last_comment_timestamp and {project_issues}.comment_count. We already have project_issue_comment() for various reasons, so this would be pretty trivial.
E.3) Exposing these 3 new columns to views in project_issue_views_data() with the appropriate filters, sorts and display. Shouldn't require any custom handlers at all, so this should be very trivial.
E.4) Fix all the project_issue views to use these new columns and handlers instead of JOINing the other tables for them.
Option (E) has tremendous appeal for a few reasons:
- No new external dependencies
- Would probably dramatically improve the performance of all the issue views
- Works for everyone running project_issue, not just d.o
- Probably only require ~5-10 hours of my time
- It would fix ORDER BY entirely, and simplify WHERE quite a bit.
It's possible that WHERE will still nail us, so we might still need to do (B) as well. Using flag would obviously be a huge win for other reasons, too. But, we need (E) either way if we want (B) to really help.
So, the current plan of attack:
nnewton is going to spend about an hour doing a trial run of approach (E). He's just going to setup a test DB, add these 3 columns to {project_issues} run some 1-time queries to populate with real data, and run an EXPLAIN on a manually constructed query to simulate what views would be doing if we go this way. That should tell us a lot about how much this helps, and if we still need to do battle with the rest of WHERE. Based on his investigation, we'll decide if we need (B) and flag or not. Either way, we'll almost certainly agree that (E) is a big win for relatively small cost, so I'll probably work on a patch for that once his experiment is done.
Phew! ;)
Comments
Comment #1
drummThe dependent subquery unnecessarily counts when all we need to know is existence:
SELECT COUNT(*) FROM comments c WHERE c.uid = 544300 AND c.nid = node.nid) > 0can be rewritten to
SELECT 1 FROM comments c WHERE c.uid = 544300 AND c.nid = node.nid LIMIT 1explain says they are the same, but my unscientific testing says the latter is faster.
Comment #2
dww@drumm: If that ends up being worth fixing, it'd be in Views:
views/modules/comment/views_handler_argument_comment_user_uid.inc
views/modules/comment/views_handler_filter_comment_user_uid.inc.
Heh, an issue search revealed this: #454754: PGSQL errors with comment_user_uid (filter and argument) which I participated in. ;) Otherwise, I don't see any issues related to this, so we should just open a new issue in the views issue queue about this aspect. Not worth us spending time on it if we're going to end up going the flag route, but worth considering if we don't.
Thanks,
-Derek
Comment #3
gerhard killesreiter commentedA) WRT per-user caching: we never have half a million logged in users at the same time. Currently about 10k in the last 24h period. Fixing the query is of course preferred, but it might be a worthwhile interim solution.
B) We planned to use flag anyway(not sure this is still on the agenda), I think this would be a very versatile approach. People could take issues out of "my issues" they no longer care about, for example. We could pre-seed the flag tables with values from the current views.
Could we find out how much better (or worse!) the flag queries would be?
C) I can't comment much on solr and how much work it'd be.
D) agreed
E) That's probably the easiest approach.
I think E is probably the way to go forward. But I'd really prefer if we wouldn't throw out B without looking at it some more, especially from the usability pov. The current solution doesn't have to be the best and just because people dislike change, that's not a reason to not change it. We should poll some heavy users of that particular view and ask them.
Comment #4
gerhard killesreiter commentedAlso: why is this part of the redesign? I sure want to see it fixed, but it seems unrelated.
Comment #5
gerhard killesreiter commentednm, saw it now. Yeah, we'll want to do something about it.
Comment #6
dww@killes: Of *course* we want to use flag for this. No one is throwing that out. I'm just wearing my "get the damn redesign launched already hat" in here. I don't want to *block* the redesign on converting the whole issue subscription UI to flag. If it turns out that the results of nnewton's experiment is that (E) alone is insufficient to make this query not kill us, then we'll go forward with flag, too. But, the hope is that (E) (which is relatively easy) will get us enough progress that we can launch like this, then circle back around and fix subscriptions with flag to make the UX vastly better and (hopefully) speed up this query even more.
Comment #7
nnewton commentedThe following is sadly mostly bad news.
I created a little script to denormalize the values we outlined above into the project_issues table. Sadly, this does very little for us. Because of the range condition MYSQL is building for this query, use of any index for the ORDER BY is impossible and the subquery, even when rewritten to be a constant index function, makes most of this a non-starter. For example, with this query written to filter entirely on project_issues:
With the subquery removed:
So ya, there isn't much we can do here. My only thought is that that subquery could possibly be replaced with a coalesced table of comment statistics. For example, isn't this information already in tracker2? I know that perhaps we don't want to make project require tracker2, but it maybe useful for a variety of reasons.
-N
Comment #8
dwwYeah, I don't want to make project_issue *require* tracker2, either. However, it seems like it should be extremely easy to patch views itself for these two handlers:
views/modules/comment/views_handler_argument_comment_user_uid.inc
views/modules/comment/views_handler_filter_comment_user_uid.inc
Directly inside those handlers, we can test for the presence of tracker2 and the {tracker2_user} table. If we've got that table, instead of this nasty:
We can just do something like this (untested, but you get the idea):
Then, we don't have to change any of the default views, we don't have to require tracker2, but any site using tracker2 will automatically get better queries for any views that use these handlers.
nnewton is going to test out this approach later today and reply here. If it looks good, I'll open a separate issue in the views queue (referencing this) about fixing those handlers when tracker2 is available.
We'll still probably need to do the denormalization I proposed in (E) in the original post, so we'll still leave this issue open for doing that...
Comment #9
dwwArgh. This is turning into quite a mess. Our experiments showed that just fixing WHERE as per #8 won't help us, since ORDER BY is still causing a filesort.
So, I tried to look into a more deep port of this view to tracker2. I started with #928110: Expose tracker2 tables and columns to views. However, even after I did a more sneaky thing (not yet in that patch) to swap out the views handler for the uid_touch argument to use a JOIN on {tracker2_user} I *still* couldn't get views to give me the query I wanted. The problem is that I need a conditional JOIN on {tracker2_user} to restrict the results by UID. However, we don't know the UID to restrict to unless we're in the argument handler. So, we can only JOIN on {tracker2_user} in the argument handler. But, we want to use the same JOIN for ORDER BY. I've got some pretty serious views fu, but I couldn't figure out how to make that work. :/
So, I fell back to this nasty hack: hook_views_query_alter()...
Ugly as hell, but it seems to work. Results in a query like this:
Which EXPLAINs like this:
p.s. Double argh! As I'm writing this comment, I realize that we don't *strictly* need the conditional JOIN as I describe above. We could also just do a dumb JOIN on {tracker2_user}.nid = {node}.nid and then restrict {tracker2_user}.uid in WHERE. Views can easily handle *THAT*. And the EXPLAIN seems equivalently happy:
So, I'm going to give that another shot before I commit myself to this views_query_alter() madness.
Comment #10
nnewton commentedHi Derek,
I just updated project_issue on http://nnewton.redesign.devdrupal.org and applied the patch. Browsing to the dashboard with devel query log on, the page loads quickly and I see this:
So a combination of around 5 ms. Very effective, these queries were well above 900ms combined even with no load on the DB server. I'm continuing to test.
This plus the other dashboard caching makes my dashboard get a first_byte in a little over 300ms.
Here are the queries before:
Comment #11
dwwOkay, here it is with using tracker2 views support instead of hook_views_query_alter(). Requires #928110-2: Expose tracker2 tables and columns to views. Seems to be working quite fine. The only minor issue I've run into while playing with this on solr.redesign.devdrupal.org is that something about the unholy conditional stuff I'm doing in the default view definition is confusing views, and even after I clear the views cache and revert the view, views still thinks it's overridden and can be reverted again. ;) Not sure what, if anything, to do about that.
Comment #12
dwwActually, no... the confusion over the view always thinking it needed to be reverted was due to #930610: Search for default views isn't restrictive and gets confused by *.view.php~ and friends. Now that that's fixed, views is very happy with my unholy conditional mojo in project_issue_user_issues.view.php. ;) Code is code...
I think it's probably more readable and sane to leave the conditional stuff directly in the default view definition file than altering our own default views via project_issue_views_default_views_alter(). So, I think #11 is basically RTBC, as soon as #928110: Expose tracker2 tables and columns to views lands....
Comment #13
dww#928110: Expose tracker2 tables and columns to views is now upstream in tracker2. However, it's only available in -dev. So, to make this conditional magic more robust, I changed the checks from:
to
so that we only try to alter this view if we've got a new enough version of tracker2 that provides views support. See attached patch.
With that, committed this to HEAD. I'll deploy as soon as I finish up #930664: Handle d.o customizations of views in code, not the DB
Hurray!
-Derek
Comment #14
dwwFYI: This is now live on d.o. The difference is dramatic. (Much rejoicing in #drupal-contribute).
Comment #15
nnewton commented/me dances with happyness.....