Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 363262-url-alias-index-D6.patch | 1.04 KB | Dave Reid |
| #23 | 363262-url-alias-index-D7.patch | 2.32 KB | Dave Reid |
| #21 | 363262-url-alias-index-D7.patch | 2.15 KB | Dave Reid |
| #20 | 363262-url-alias-index-D7.patch | 2.21 KB | Dave Reid |
| #18 | 363262-url-alias-index-D7.patch | 1.63 KB | Dave Reid |
Comments
Comment #1
catchNot sure how much real difference this actually makes, so demoting from critical until I can do some proper benchmarks.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe 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.
Comment #3
webchickI'd like to get at least one review by someone who didn't (co-)write the patch. ;)
Comment #4
chx CreditAttribution: chx commentedMakes a lot of sense to me...
Comment #5
webchickI'm pretty sure that update is going to fail, since it's missing return $ret;
Comment #6
chx CreditAttribution: chx commentedComment #7
chx CreditAttribution: chx commentedComment #8
catchWe 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.
Comment #9
webchickRight. 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.
Comment #11
catchDidn't apply properly to D6, here's a backport.
Comment #12
Gábor HojtsyCommitted to Drupal 6. Happy figuring out with the Drupal 7 upgrade path.
Comment #13
Dave ReidIt'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.
Comment #14
Dave ReidLast patch had an extra newline.
Comment #15
Gábor HojtsySince 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...
Comment #16
Dave ReidGabor, 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...
Comment #17
Gábor HojtsyDave: 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.
Comment #18
Dave ReidAh, 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.
Comment #19
Gábor Hojtsysystem_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.
Comment #20
Dave ReidThanks again Gabor. Ok this one should be ready.
Comment #21
Dave ReidHmm... 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"Comment #23
Dave ReidRe-rolled for the changing update functions.
Comment #24
Dries CreditAttribution: Dries commentedReviewed and tested. Committed to CVS HEAD for inclusion in Drupal 7. Setting the version to Drupal 6.
Comment #25
Dave ReidRevised 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.
Comment #26
andypostis this patch needed if there's #215080: Performance: change {system}.type: alter table system modify column type VARCHAR(32);
Comment #27
Dave ReidThis is a different issue. Marking as RTBC as there is literally no code change.
Comment #28
Gábor HojtsyThanks, committed the phpdoc cleanup from #25 as well.