Improper table structure for url_alias

Indeed. Bumping to Drupal 7: we fix bugs in the current development version first, before backporting them.

Here is a patch

Indeed-ly. Committed to CVS HEAD.

This change come from #358315: drupal_lookup_path() not respects alias' order

So rollin back this change should be measured!

It looks from #358315: drupal_lookup_path() not respects alias' order like pid was originally added to the index in an attempt to avoid a filesort, but given that it is already a primary key that doesn't seem necessary? And the current queries in core certainly don't seem to have performance problems in that regard:

mysql> EXPLAIN SELECT source, alias FROM url_alias WHERE source IN ('node/1') AND language IN ('en', 'und') ORDER BY language ASC, pid ASC;
+----+-------------+-----------+------+-----------------+-----------------+---------+-------+------+-------------+
| id | select_type | table     | type | possible_keys   | key             | key_len | ref   | rows | Extra       |
+----+-------------+-----------+------+-----------------+-----------------+---------+-------+------+-------------+
|  1 | SIMPLE      | url_alias | ref  | source_language | source_language | 767     | const |    1 | Using where | 
+----+-------------+-----------+------+-----------------+-----------------+---------+-------+------+-------------+
1 row in set (0.01 sec)

mysql> EXPLAIN SELECT alias FROM url_alias WHERE source = 'node/1' AND language IN ('en', 'und') ORDER BY language DESC, pid DESC;
+----+-------------+-----------+------+-----------------+-----------------+---------+-------+------+-------------+
| id | select_type | table     | type | possible_keys   | key             | key_len | ref   | rows | Extra       |
+----+-------------+-----------+------+-----------------+-----------------+---------+-------+------+-------------+
|  1 | SIMPLE      | url_alias | ref  | source_language | source_language | 767     | const |    1 | Using where | 
+----+-------------+-----------+------+-----------------+-----------------+---------+-------+------+-------------+
1 row in set (0.00 sec)

mysql> EXPLAIN SELECT source FROM url_alias WHERE alias = 'test' AND language IN ('en', 'und') ORDER BY language DESC, pid DESC;
+----+-------------+-----------+------+----------------+----------------+---------+-------+------+-------------+
| id | select_type | table     | type | possible_keys  | key            | key_len | ref   | rows | Extra       |
+----+-------------+-----------+------+----------------+----------------+---------+-------+------+-------------+
|  1 | SIMPLE      | url_alias | ref  | alias_language | alias_language | 767     | const |    1 | Using where | 
+----+-------------+-----------+------+----------------+----------------+---------+-------+------+-------------+
1 row in set (0.00 sec)

So from a performance perspective I think the change committed here was OK.

However, there are other considerations for why we might roll it back. The patch committed here overall seems like an API change, and it's not clear what bug it was fixing. I don't know why we need to prevent multiple sources for the same alias/language at the database schema level, given that:

1. We already have code in core to specifically deal with that possibility (as can be seen from the last query pasted above, the behavior is to sort by pid DESC so as to use the most recently-added alias in case of a conflict). That code servers no purpose if we prevent it at the database level, so at a minimum this patch should have modified those queries.
2. We have an API function path_save() that allows you to save whatever you want to the database, but now the schema is inconsistent with the functionality - i.e., you can get some nice big errors in your code if you save an alias without checking for others "like it" first.
3. In Drupal Gardens we are seeing upgrade failures on some sites as a result of this change. That is a HEAD to HEAD upgrade so of course it is unsupported. I tried to see if there was a legitimate way - i.e., starting with a Drupal 6 site - to reproduce that situation, but haven't found one yet. However, given that the code which allows those duplicate aliases in the database has been in Drupal 6 for several months now as a result of #358315: drupal_lookup_path() not respects alias' order it is theoretically possible that there are Drupal 6 sites which wound up in that situation. Those hypothetical sites would fail when trying to upgrade to D7, unless we wanted to write some update code to deal with that situation and fix it.

Unless there is a good reason for the database schema change that was not explained above, then I think the simplest thing to do here is to roll this back for Drupal 7 and maybe consider it again for Drupal 8.

Instead of a complete rollback, we could maybe do the attached (only lightly tested) patch.

This continues to leave 'pid' out of the indexes - since it doesn't really make sense there - but changes the unique key to a regular index in recognition of the fact that in Drupal 6 at the moment the inclusion of 'pid' means that it is not actually meaningfully unique anyway. Thus, we stay consistent with Drupal 6 and do not need to deal with any of the potential code changes I mentioned above.

No, we certainly don't want to roll the change back. As indicated in the original post, there is a structural issue in url_alias: we define a unique key on 'alias', 'language', 'pid', which is *completely useless* because pid is already unique by design.

So we probably want just to remove that constraint and live an happy life ever after.

Cross-posted with #6.

This said, we actually want the pid to be part of the index, because of the sort on language, pid.

Well, pid is already a primary key, and the EXPLAIN queries above show that there is no filesort even without it being part of the index. (Also, in the typical case, the sorting by pid probably won't even need to happen, or at least only happen to a very small number of rows.)

Do you think there is still a big advantage to explicitly making it part of the index?

@David: because the conditions (the WHERE part) return only one row, MySQL doesn't have to sort, so it will not even care in the examples you copied. Try to remove the UNIQUE key and add more data, and you will see the filesort reappearing.

I am still not reproducing a filesort, even with no unique key and more data. If I apply the patch from #6 and reinstall Drupal, then do the following, there doesn't appear to be a filesort:

mysql> INSERT INTO url_alias (source, alias, language) VALUES ('node', 'test', 'und');
Query OK, 1 row affected (0.01 sec)

mysql> INSERT INTO url_alias (source, alias, language) VALUES ('node/1', 'test', 'und');
Query OK, 1 row affected (0.00 sec)

mysql> INSERT INTO url_alias (source, alias, language) VALUES ('node/2', 'test', 'und');
Query OK, 1 row affected (0.00 sec)

mysql> INSERT INTO url_alias (source, alias, language) VALUES ('node/1', 'test1', 'und');
Query OK, 1 row affected (0.00 sec)

mysql> INSERT INTO url_alias (source, alias, language) VALUES ('node/1', 'test2', 'und');
Query OK, 1 row affected (0.00 sec)

mysql> SELECT source FROM url_alias WHERE alias = 'test' AND language IN ('en', 'und') ORDER BY language DESC, pid DESC;
+--------+
| source |
+--------+
| node/2 | 
| node/1 | 
| node   | 
+--------+
3 rows in set (0.00 sec)

mysql> EXPLAIN SELECT source FROM url_alias WHERE alias = 'test' AND language IN ('en', 'und') ORDER BY language DESC, pid DESC;
+----+-------------+-----------+------+----------------+----------------+---------+-------+------+-------------+
| id | select_type | table     | type | possible_keys  | key            | key_len | ref   | rows | Extra       |
+----+-------------+-----------+------+----------------+----------------+---------+-------+------+-------------+
|  1 | SIMPLE      | url_alias | ref  | alias_language | alias_language | 767     | const |    1 | Using where | 
+----+-------------+-----------+------+----------------+----------------+---------+-------+------+-------------+
1 row in set (0.00 sec)

unique key make no sense with pid within.

Another issue that LANGUAGE_NONE is 'und' so idea "...The language should have priority over the empty language." is totally broken because there are different languages

I think query should LIMIT 1 because static cache of drupal_lookup_path() stores only first row

I don't know any place in the code where we do a query on this table sorting by language,pid which doesn't have a WHERE clause attached to it. But even so, and index on this table is probably not a big deal as inserts aren't usually a performance concern anyway.

Here's a patch which includes the pid field.

OK, fine with me - I agree it doesn't hurt much to have it there.

However, if we're keeping pid in the alias index, we need to keep it in the source index also; they are parallel. See attached patch.

So basically, after this patch, the end result of the D6->D7 update would be only one meaningful change to the url_alias table: The unique key in D6 (which was not actually providing any meaningful uniqueness constraint) becomes an index, and everything else stays the same.

works for me.

@JacobSingh take a look at drupal_lookup_path()

Suppose better to incorporate this with system_update_7042()
And remove system_update_7056() as useless also making upgrade process a bit quicker

@andypost, the queries in drupal_lookup_path() are the ones Jacob was looking at. None of them appear to cause an issue.

Anyway, I agree with you about system_update_7042() - the attached patch does that. We don't seem to be renumbering update functions when we take them out (there are variety of gaps all over the place at the moment) so I didn't do any renumbering related to removing system_update_7056() either.

Does this look good?

I created a (preemptive) HEAD to HEAD patch for this here: #863184: Update for improper {url_alias} table structure

Great, but whats about #12 - priority of aliases? separate issue?

+1 RTBC #17

Sounds to me like a separate issue, yeah.

I file separate issue #863318: Wrong sort order of aliases for different languages

Committed to CVS HEAD. I'm moving this to HEAD 2 HEAD so we can look at it in that context just to make sure.

@Dries please change your workflow to create new issues in head2head.

#863682: Handle Improper table structure for url_alias created in HEAD2HEAD

Thanks for committing.

@Dries please change your workflow to create new issues in head2head.

Especially since in this case, I already created the Head2Head issue, linked to it above (in #18), and already had a patch posted there :)

I don't understand why the uniqueness on alias+language was removed.

Obviously an internal path can have any number of aliases. But is the same alias really allowed to point to more than one internal path per language? And what does it mean if it does?

If a try to create a duplicate alias on admin/config/search/path/add, I am blocked by the following code in path_admin_form_validate() that seems to indicate that alias+language is supposed to be unique:

  $has_alias = db_query("SELECT COUNT(alias) FROM {url_alias} WHERE pid <> :pid AND alias = :alias AND language = :language", array(
      ':pid' => $pid,
      ':alias' => $alias,
      ':language' => $language,
    ))
    ->fetchField();

  if ($has_alias) {
    form_set_error('alias', t('The alias %alias is already in use in this language.', array('%alias' => $alias)));
  }

pid was added to the unique key in #358315: drupal_lookup_path() not respects alias' order for performance reasons. The patch added ORDER BY pid DESC to two different queries, but AFAICT it was only necessary for one of them. For the other one (shown below) there was no need to order by pid too, because dst+language were already unique.

-        if ($src = db_query("SELECT src FROM {url_alias} WHERE dst = :dst AND language IN(:language, '') ORDER BY language DESC", array(
+        if ($src = db_query("SELECT src FROM {url_alias} WHERE dst = :dst AND language IN(:language, '') ORDER BY language DESC, pid DESC", array(

Do you agree, or did I misunderstand something?

@c960657, the same alias pointing to multiple sources is blocked via the UI, but still handled in path.inc (it's allowed via the API).

And as described above, it didn't seem worth adding this restriction to the database schema this late in the cycle (and dealing with any potential update headaches this might cause). It might be a good idea for Drupal 8, though.

Given that, I'm not sure if you're still saying there is some way we could improve the existing queries in Drupal 7 (it sounds like there might be)... probably better to open a separate issue for that?

No, I think the existing queries are fine, given the current database schema.

I know it's late in the cycle, but on the other hand it's one of those problems that get harder to fix, the longer you wait. I think it's a regression, but that regression was introduced in D6 as well, so I guess this is not a blocker. I have created a new issue for this: #925474: Improve table structure for {url_alias} #2.