Missing index on url_alias table
catch - January 23, 2009 - 17:43
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | path.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | needs backport to D6, Performance |
Description
HEAD:
#
mysql> EXPLAIN SELECT dst FROM url_alias WHERE src = 'user/1590' AND language IN('en', '') ORDER BY language DESC;
#
+----+-------------+-----------+------+---------------+------+---------+-------+------+-----------------------------+
#
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
#
+----+-------------+-----------+------+---------------+------+---------+-------+------+-----------------------------+
#
| 1 | SIMPLE | url_alias | ref | src | src | 386 | const | 2 | Using where; Using filesort |
#
+----+-------------+-----------+------+---------------+------+---------+-------+------+-----------------------------+With the patch:
mysql> EXPLAIN SELECT dst FROM url_alias WHERE src = 'library' AND language IN('en', '') ORDER BY language DESC;
+----+-------------+-----------+------+------------------+--------------+---------+-------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-----------+------+------------------+--------------+---------+-------+------+-------------+
| 1 | SIMPLE | url_alias | ref | src,src_language | src_language | 386 | const | 1 | Using where |
+----+-------------+-----------+------+------------------+--------------+---------+-------+------+-------------+I spotted the filesort, Damien noticed the missing index.
On my drupal 6 site, I'm getting queries upto 25ms (most around 1-2ms) in drupal_lookup_path() on a table with around 30k aliases. With the patch, everything is < 1ms.
Patch is against Drupal 7, but modifies the Drupal 6 updates - since if this goes in at all it'll need to be backported.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| url_alias_index.patch | 992 bytes | Idle | Failed: Failed to apply patch. | View details |
#1
Not sure how much real difference this actually makes, so demoting from critical until I can do some proper benchmarks.
#2
The simple (src) alias was good for D5, but should have been extended to (src, language) when D6 introduced the new language subsystem.
This can quickly become critical on a site with many aliases that change frequently.
#3
I'd like to get at least one review by someone who didn't (co-)write the patch. ;)
#4
Makes a lot of sense to me...
#5
I'm pretty sure that update is going to fail, since it's missing return $ret;
#6
#7
#8
We don't have a way to check if an index exists so summary of irc discussion:
Roll this for D7 with a system_update_7*
Backport to D6 with a system_update_6*
Let Crell, chx and Damz sort out a way of checking whether the 6.x update has run or not before we release.
Here's a straight D7 patch.
#9
Right. chx brought up "What happens if someone upgrades to Drupal 7 from Drupal 6.5?" the answer is, their database would get out of sync because the update would never run.
For now, I've committed #8, and left RTBC for D6 on #7. When this is committed to D6 (assuming it is), please kick back to 7.x and mark CNW so we don't forget to address this "same update might run more than once" thing in one way or the other.
#360854: db_index_exists() missing, module updates cannot handle indexes properly introduces a db_index_exists() function which is probably a good pairing with db_table_exists() and db_column_exists() to deal with this, but still needs work and review. We also desperately need to fix #363911: Batch API needs to return useful error messages.
#11
Didn't apply properly to D6, here's a backport.
#12
Committed to Drupal 6. Happy figuring out with the Drupal 7 upgrade path.
#13
It's easy to port this back to D7. We just rename our original update 7018 to 6049 and shift all the other 7018+ updates down one.
#14
Last patch had an extra newline.
#15
Since by tradition, Drupal 7 is expected to drop all pre Drupal 6.0 update functions, it would be important to mark this as being post 6.0. System update 6047 was the last in Drupal 6.0, so the ones from there should be kept when Drupal 7 is released. A marker should be put in that place in both Drupal 6 and 7 (or previous updates should already be removed from Drupal 7).
Anyway, I think since we are moving back the updates here, this patch should add that note in.
Ref: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.inst...
#16
Gabor, I really have no idea what you're talking about. :/ We're supposed to remove any 5xxx udpates in 7.x since those are two versions behind. We should be keeping the 6xxx updates. If we simply rename the update from 7018 to 6049 in 7.x, if a user upgrades to Drupal 7 from Drupal 6.9, the update will still fire. Not sure why we need markers...
#17
Dave: well, D6 shipped with with all D5 updates minus the updates added after D 5.0 removed. See http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.inst... again. It has a "@defgroup updates-5.x-extra Extra system updates for 5.x" and then starts off the 5 -> 6 upgrades. Drupal 7 is missing the updates-6.x-extra marker which will be used to cut off all update functions before it before the 7.0 release.
#18
Ah, I've got it now. Thanks Gabor. I haven't had my coffee yet so I'm not quite awake yet apparently. :)
So we are still going to need to adjust the 6.x system.install again aren't we? I'll file a new issue to remove all the 5.x to 6.x update functions.
#19
system_update_6048() was also an extra as far as my research above found. So let's move the marker before that as I've said in #15.
#20
Thanks again Gabor. Ok this one should be ready.
#21
Hmm... seems like I should actually move the line "The next series of updates should start at 7000." to
@} End of "defgroup updates-6.x-extra". It doesn't make sense to have that line with the@} End of "defgroup updates-5.x-to-6.x"#22
The last submitted patch failed testing.
#23
Re-rolled for the changing update functions.
#24
Reviewed and tested. Committed to CVS HEAD for inclusion in Drupal 7. Setting the version to Drupal 6.
#25
Revised patch for D6. The update function is already there, just adding/moving some doc blocks to show where the 5.x->6.x updates stop and the extra 6.x updates begin.
#26
is this patch needed if there's #215080: Performance: change {system}.type: alter table system modify column type VARCHAR(32);
#27
This is a different issue. Marking as RTBC as there is literally no code change.
#28
Thanks, committed the phpdoc cleanup from #25 as well.
#29
Automatically closed -- issue fixed for 2 weeks with no activity.