COUNT(*) is an expensive query in InnoDB.

It is not really that expensive (at least on MySQL with MyISAM). On a site with 96,000 aliases, it takes 20 to 40 milliseconds to execute this query.

But the idea here is to ask : "are there aliases". Not just : "is path module installed".

And with every new alias the count query gets longer and longer and longer. I removed the query (which is run on every new page) which was taking nearly 30ms to run and improved my site performance. You should only be able to have an alias if and only if the path module is installed, correct? And while I'm counting those rows someone else waits for the DB. IMO we should not really care if there are zero aliases and just query url_alias table regardless. Counting rows is expensive; maybe not on its own single occurrence, but it adds up which makes it expensive.

See plenty of discussion at http://drupal.org/node/106559

Not a duplicate since the count still exists.

Earnie

Still, the path module can be enabled, but no aliases in the database.

It is best to create a proper patch, with benchmarks of before and after. This way, it is more concrete.

Still, the path module can be enabled, but no aliases in the database.

But the user of the path module should not care if there are currently zero in the database vs 100K within this function. It is something which isn't useful and takes more time to do the count than to just do the select request to begin with.

It is best to create a proper patch, with benchmarks of before and after. This way, it is more concrete.

I plan to do just that. I created a support request for it first to get comment. I will change to a bug report when I have a patch to apply.

Another approach is this:

- Cron would do the select count(*) every so often, and store whether there are aliases in a variable_set().
- The code that now checks count just checks that variable.

I have thunked a solution of

How should the LIMIT 1 be specified since it is MySql specific? (Hmm, guess it is ANSI specified. I thought Oracle didn't support it though.)

Here's the HEAD patch.

And the same patch rolled for 6.x and 5.x.

IMHO a SELECT ... LIMIT 1 answers the question "is there an alias in the db" way more efficiently than counting every alias out there. It may be "only 30-40 ms", but its still an improvement and as earnie said, the more alias you got, the worse the problem. +1 from me.

Any benchmarks for the new patch?

It would be interesting to benchmark it in both MySQL MyISAM, INNODB and Postgre with lots of nodes if possible. MyISAM has optimization for COUNT(*), so there might not be a terrible difference there.

I went from 30+-ms to between 0.29ms and 0.45ms on InnoDB.

Thanks Earnie, that confirms what I've been thinking; this probably got overlooked because COUNT(*) is extremely fast in MyISAM tables. For everyone else (InnoDB and Postgre), this way of doing things is very bad performance wise (when the table is big).

To Count Or Not To Count

I did the patch before I found this.

Not so fast ...

Turn the MySQL query cache off (or restart MySQL between tests) so that it does not skew the numbers.

I did some benchmarking of MyISAM and InnoDB on a table with 10,438 nodes.

The results DO NOT SHOW that LIMIT is faster than COUNT.

Here is the script used to test:

Here are the results.

Since MyISAM is in wide use, your patch will cause slowdowns on many sites.

kbahey: your script have an extra ' at the end in the 3 queries except the original COUNT(*) on MyISAM, which would cause the query to fail entirely. Are you sure the queries ran properly?

[edited -- kbahey]

Fixed.

Here are the results after the fix.

For PostgreSQL with 10,000 nodes in the table, it says:

Seems ready to be committed to me. It serves three database engines well.

This still applies cleanly. But if the LIMIT is slower on MyISAM, why not the variable_set in cron proposed by kbahey in #7? This could also be checked/set in path_set_alias() for the very rare case that a site has 0 aliases then gets one in between cron runs. Checking the variable has to be quicker than this query on every page.

For the obvious reason that if someone turns on path aliases they expect it to be available at the time they submit the page and not after waiting for the length of time that the cron timer expires to execute it. And as I've already said, the count isn't important, the fact that we have aliases to serve is.

The whole idea of protecting against having rows in path_alias is debatable since we wouldn't have path turned on if we weren't going to use it. I'm one who likes to eliminate such frivolous things and just let the code handle it just like it would for no alias found. It makes the code smaller and therefore faster, uses less memory and easier to maintain. It's a module and if you're not using it then disable it to make the system faster. I submitted this patch because it is the least intrusive to the existing code.

earnie:

This could also be checked/set in path_set_alias() for the very rare case that a site has 0 aliases then gets one in between cron runs.

Or for that matter, could be set on saving and deleting path aliases plus enabling path module - and left both out of both page requests and cron entirely no?

Catch, why should http://api.drupal.org/api/function/drupal_lookup_path/6 care about count? How does having a count help for this function? I understand that if the path alias module isn't used that perhaps the function should just return false; but if it is used how is count useful? What you propose is adding extra overhead for no gain.

earnie: I think we're talking at cross-purposes.

What I'm saying is keep the altered query from #10, but move it to a variable_get/set in path_set_alias() when paths are added and deleted rather than read. Then all burden goes to writes and it's just if ($has_path) in drupal_lookup_path() itself.

This is the same as kbahey suggested, but without the lag of relying on cron, and any overhead is only on writes. I don't see how this adds extra overhead to drupal_lookup_path() - it even simplifies it and it should gain slightly more than the original patch as well no? Or am I simply wrong here?

I understood

What I'm saying is keep the altered query from #10, but move it to a
variable_get/set in path_set_alias() when paths are added and deleted
rather than read. Then all burden goes to writes and it's just if
($has_path) in drupal_lookup_path() itself.

you were saying this. My question is why do we care about the count? Why modify the updaters to keep track of a count? Where is it important to have?

Because a small performance hit on some updates update and one additional variable is a tiny price to pay for removing that query altogether on every page load. Since neither LIMIT 1; and SELECT COUNT() are optimal cross-databse, it seems like a win-win to me to just check the variable instead. You don't need to store the count, just a TRUE/FALSE as to whether there's any rows in the path table and update only if it changes.

@catch: So how do you determine when you've removed the last path to set that variable back to FALSE? Am I missing somewhere where there's a REMOVE * and you'd therefore know that you were for sure back to no rows?

The world mostly runs MyISAM (edit: might be that every table engine) which checks against the table definiton (edit: the frm file) and not against the table data when running a count of all rows.

So. it's not the FRM file but the MyISAM table header. Sorry. Anyways -- it's extremely fast , it's pre stored always. http://www.browardphp.com/mysql_manual_en/manual_Storage_engines.html

Susurrus: well you could run the query just after path deletion to ensure you catch if there's zero rows. Or if using COUNT() and $count -1 = 0 just before a delete, then you'd know that way as well.

But, isn't the point to remove queries? Why would we want to run COUNT() as a separate query?

As far as this query, it should be very quick since pid is a primary key on the url_alias. In theory, the number of rows in this table is stored directly by the database (I would imagine this would be the case for all databases) and the query would run very quickly. Has anyone actually benchmarked the cost of this query per page load?

Since the reason for needing to have a count hasn't been answered to any satisfaction, I will submit a different patch soon that just checks for the path module existing. If I have path module enabled but no aliases created yet is minimal and I wouldn't care that the lookup for a non existent alias occurs with zero rows in the table.

@Susurrus: I submitted the original patch because the devel module identified the count as slow. This query is executed on every page so eliminating long unneeded queries is important. Moving the query elsewhere isn't feasible either. I'm creating aliases for every tag I create, not to mention every page title. See the statistics at #20 for differences between myisam, innodb and postgres.

@chx: While myisam is good for small sites IMO, I am processing 100K nodes (soon I will be processing more than 500K nodes) in batch and I need transactions. I chose innodb because I deemed that since it was still mysql I would have fewer problems since I really am not familiar with postgres.

earnie: Oh, now I see! Checking for path module rather than in the table seems completely fine to me, sorry! I was still working on the basis of #10.

As promised but this patch only works if http://drupal.org/node/222109 is also applied.

I think it is OK to remove that query -- not everyone uses MyISAM. However, I don't like the static $has_path that was introduced. module_exists() should be fast -- if it is not, it should be made faster. Isn't module_exist() caching this already?

That is what I almost did. The module_exists function sets a variable calling module_list>module_list and then calls array_key_exists. It may be slightly faster to cash the result of module_exists. I've attached a modified patch without the cache.

OK, I've committed this to CVS HEAD. Thanks.

Dries: As I stated at http://drupal.org/comment/reply/196862#comment-731300 you also need to commit http://drupal.org/node/222109 or it doesn't work because the module_list cache isn't filled.

Automatically closed -- issue fixed for two weeks with no activity.

This patch seems to have broken URL aliases entirely; see #237336: Url Aliases no longer work. (I tried applying the suggested companion patch at http://drupal.org/node/222109 which @earnie recommends, but that didn't seem to help.)

I don't think the problem is the "fault" of this patch, but seems like it might be a deeper problem that this patch happened to expose... so it's probably best to discuss it over at the new issue.

IMO this patch is at fault and I think it needs to be rolled back.

I'm not going to mess around with issue statuses, so rollback patch is here: http://drupal.org/node/222109#comment-809566

I'm pretty certain it was indeed the fault of this patch, so I'm reopening this issue and marking http://drupal.org/node/222109 as duplicate.

The short version is path module doesn't exist during DRUPAL_BOOTSTRAP_PATH because it doesn't implement hook_boot or hook_exit, so checking for it always returns FALSE.

This is a simple rollback so I'm marking RTBC - the patch is just a reversal of #38 rolled from root for convenience.

Once that's done, we should keep this issue open at CNW and try to prevent the query another way.

Simply rolling back a fix that showed issues elsewhere isn't fixing the real problem. Please review the patches by David Rothstein at #222109: module_list() is broken comment #10.

If the goal is to get URL aliases working again quickly, then I agree that rolling back this patch is probably the best move.

However, as @earnie said, we have reopened http://drupal.org/node/222109, since there is indeed a more fundamental problem discussed there that is related to this one... and fixing it may allow the current patch here to work eventually (it seems that it turned out to be more complicated than I originally thought, though).

Hm, even if we can get the patch in #38 to work, is module_exists('path') really the right thing to check?? What if you create some aliases but then later disable the path module... won't that lead to a problem with broken links?

Currently, Drupal checks URL aliases in the database REGARDLESS of whether the path module is enabled... the path module itself basically just controls the administrative interface. I think in many ways that makes more sense.

Adding to my issue queue (catch thought I should have a look at it, so... ;)).

Hm, even if we can get the patch in #38 to work, is module_exists('path') really the right thing to check?? What if you create some aliases but then later disable the path module... won't that lead to a problem with broken links?

My original patch at #9 took care of this. However, if someone (me for instance) disables the path module don't you think they might just think that path aliases are no longer valid? See Dries comment at #37, its obvious from the remark that he was thinking that module_exists('path') should work for this.

Well, it's not wrong, I guess, just drastic. You click on that little checkbox and suddenly a bunch of links all over the Internet are broken ;)

If this patch goes in then maybe part of the solution is to rewrite some of the help text for path.module. Right now, the help text basically just talks about the admin interface for managing aliases (as it should, since in Drupal 6 that's all it controls, not the aliases themselves). For example, the description in the module list is currently Allows users to rename URLs, and the help text has things like:

I guess this is somewhat related to @catch's comment here, that path.inc and path.module are perhaps more tangled up than they should be.

I agree with David -- let's roll back and fix the help text as a stop gap solution, then spend the necessary time to work on a proper solution. Care to re-roll with the documentation suggestions?

As per Dries's request, setting to CNW.

@Dries: Please don't forget to look at the synergies around #222109: module_list() is broken to help module_list. Chx has a patch being reviewed now. This simple patch has brought to light some inconsistencies with the use of module_list that will make D-7 even better. Let's not roll back just yet but wait for the outcome of #222109: module_list() is broken before a decision is made.

fwiw I'm not sure the help text needs changing on this, or if it does I'm not sure in what way.

With the rollback, the path module does exactly what it says it does - provides the interface for adding and deleting paths. So the current help text would be correct. It's only with the change from the query to module_exists() as it currently is in HEAD that the path module influences whether paths themselves are activated or not.

I'm fine with leaving things as they are until module_list is reworked (assuming that gets in quickly), in which case we'd just need a patch to update the help text once the other issue is fixed. If we want to roll this back and revisit it again from scratch, then the current patch should do that fine, so marking back to CNR.

http://drupal.org/node/196862#comment-809836 still applies, still fixes the issue, makes all path tests pass.

http://drupal.org/node/259412 is the current issue addressing with the root problem, but IMO this shouldn't be postponed for a complete module system revamp.

I'm marking this back to RTBC again.

Yeah the module revamp will fix it but until then why keep testing back?

Committed to CVS HEAD. Thanks.

Changing status so we can revisit this when the module revamp is in.

It's easy, really.

With less tpyos ;)

If I read this correctly, it makes the path system not function unless the path module itself is installed. Who's to say the core path module is the only possible way to configure paths? What if I want pathauto (or similar) running, but not have the core path module's UI? What if I want to provide my own API instead? This is like making Views not function unless Views UI is enabled, but keeping Views UI disableable. -1.

Found one more count and re-read the whole issue. If you are using aliases without the path module then set the variable too, it's easy.

Hm, at a minimum wouldn't this need to be accompanied by a change to the path.module's help text which explained that disabling the module breaks all your existing URL aliases?... However, I think @Crell is exactly right. Ideally, path.inc should be an API which works on its own, with path.module being one possible UI for accessing it (just like the separation between menu.inc and menu.module).

However, what if instead of making the URL aliases disabled by default, you made them enabled by default, as in the attached patch? There could then be a radio button for disabling path aliases altogether at admin/settings/performance (or even if not, the variable would still be available to set in settings.php for sites that cared about it).

Latest patch in #73660: Allow path module to be uninstalled uses module_exists(). Setting the variable works too though. Not sure which I prefer.

This one needs a hook_update_N() to set the variable on existing sites regardless, so CNW for now.

Never mind my patch... I wasn't thinking straight about this and didn't realize the point was to get rid of the count query in all cases :)

However, I think what I was saying in the comment still makes sense. If this is a separate variable, it makes sense for path.module to enable it in path_enable(), but not to force it to be disabled in path_disable().

What is wrong with just:

The current way of doing this (ie. not relying of the path module being enabled, which makes little sense), is the good one imho.

Damien, that's a query, though fine enough that i used it in the update function. I want to avoid a separate query if at all possible. I have removed the path_disable function. Once again, if you provide a path-equivalent module then add a variable_set to your _enable.

Damien, that's a query, though fine enough that i used it in the update function. I want to avoid a separate query if at all possible. I have removed the path_disable function. Once again, if you provide a path-equivalent module then add a variable_set to your _enable.

Trying to remove one straight-forward query has very little value by itself.

If you really want to remove if in the most probable use case, why not "simply" issue a check query if the first lookup fails?

The last submitted patch failed testing.

Is there an API call for setting a path? Why not just magically regenerate the "there are aliases" variable after each set/delete? If there are some, presumably we want to use the alias look up system. If not, disable it entirely. And if there's no API function and it's just a direct query, FAIL! Add it. :-)

A better API seems like it would be a big win in its own right, and doing the variable_set/variable_del in there is more or less what @catch proposed way up above, I think.... but it might have to be a pretty comprehensive API in order to work for this. For example, a module like Pathauto currently runs queries like DELETE FROM {url_alias} WHERE src LIKE '%s%% in order to support bulk deletion by alias type, and if that kind of query can't be done via the API, it seems like maybe the variable could get out of whack?

Maybe the simplest approach would be to provide a variable, but not bother trying to set it at all. Just leave it as something that people can optionally set in settings.php if they care about ultra-high performance and really want to avoid this single DB query on every page load. For example, look at the approach in http://api.drupal.org/api/function/drupal_is_denied/7 - something like that...

We need a universal solution to the COUNT(*) problem. We just made InnoDb the default engine, so we can ignore it no longer. If you think it is a minor issue, grep the source or the query log and you find several of these queries on every page. And sometimes on big tables like url_alias and node_access.

I agree with an earlier proposal that modules should calculate and store row counts during cron. They can do that using a new core table or with separate variables.

Based on http://dev.mysql.com/doc/refman/5.0/en/innodb-restrictions.html

To get a fast count, you have to use a counter table you create yourself and let your application update it according to the inserts and deletes it does. SHOW TABLE STATUS also can be used if an approximate row count is sufficient.

Setting a new title.

The attached patch goes part of the way: it removes "useless" COUNT(*) queries, ie. queries where we only want to know if the count is non zero. It's probably a good start to commit that to D7, and then to backport to D6.

@Damien - see #18 where kbahey bechmarked this - not promising.

The last submitted patch failed testing.

Re: #76: Yes, which is why MySQL suggests http://dev.mysql.com/doc/refman/5.0/en/show-table-status.html for those tables we just want to know data exists. This can be used for MyISAM as well, it isn't specific to InnoDB. We probably need some API db_table_has_rows($table) or some such that returns boolean. Then other DB vendor can do the right thing.

Looks like we've got a choice between a SHOW TABLE STATUS and variable_get('path_table_has_rows', FALSE); then. Anyone able to run benchmarks on SHOW TABLE STATUS?

Marking as duplicate of #464164: path.inc your time has come. You can follow up on that issue instead to track its status. If any information from this issue is missing in the other issue, please make sure you provide it over there.

However, thanks for taking the time to report this issue.

This issue has real discussion and real benchmarks. That issue no proper writeup, no followups, and is newer. Removing dupe status.

Priority = Critical since we now ship with InnoDB.

#464164: path.inc your time has come removes path.inc and makes URL aliases optional. Proper fix for the real, underlying cause.

This patch touches the surface only.

Subscribing...

@sun: I'd say this issus has moved well beyond the scope of the original patch/description.

Agreeing with catch @ #79, need some tests on 'SHOW TABLE STATUS'.

and with ernie @ #78, I think for developers and and maintenance tasks the db_table_has_rows() or db_table_empty() solution would be appreciated, but alot of time we actually are counting so that won't fix everything.

At first I was thinking of this new 'count' table as a pain similar to the 'sequence' table and way it used to be with nids. Still thinking about consequences of direction taken here now that InnoDB will be default engine.

I stand by my patch in #75.

I used the following script on a 350,000 rows table (a copy of drupal.org node table):

and I'm getting:

No comment.

Let's try again.

The last submitted patch failed testing.

Thanks you, testing bot!

Benchmarks in #85 are compelling. Bot is happy. RTBC.

The benchmark in #85 was not conclusive for comparing COUNT(*) and LIMIT 1 on MyISAM. Here is a typical result with $count = 100000:

It seems that COUNT(*) is consistently a tiny bit slower (in a 1-5% range) then LIMIT 1 on MyISAM, while LIMIT 1 are equally costly on MyISAM and InnoDB.

I spent a good amount of time reviewing this patch, reading the linked resources, etc. It seems like there's a good bit of confusion and cross-talk which was a bit confusing, but here's where I think we stand:

There are sometimes legitimate reasons to use COUNT(*) to provide a count of the rows in a table. These queries are prohibitively slow on InnoDB (and really, anything but MyISAM, which is specifically optimized for this case). In this case you need a summary table, like we do with node_comment_statistics.comment_count. Or, alternatively, a variable in the variables table as chx's attempts at this patch did.

However, in the *vast* majority of cases, all we are using COUNT(*) for is "Are there rows in this table?" There is no point in running a COUNT query on a table that's getting bigger... and bigger.. and biiigggggeerrrrr.... if that's all you want to know. Damien's patch addresses these queries by changing them to just a simple 'SELECT 1 FROM ... LIMIT 1'. This makes a lot more sense, since the query is expressing what you actually mean. I'm a bit concerned about the verbosity and complexity of these queries (db_query_range, along with a cast to bool), and discussed with Damien whether a wrapper function might make sense, but he pointed out that there's really nothing these queries have in common other than that, so it would just be a needless layer of indrection, really.

Others in this issue have attempted to remove the path.inc query altogether by various means (just checking for whether path module is enabled, which is a no-go because then URL aliases would break if the optional Path module were disabled, updating a variable on cron which can be checked at read time, ec.) but none were very optimal, and none apply across the board to all of the COUNT(*) queries in core. I think that Damien's approach is best of the presented options.

However, what's a little confusing to me is that past benchmarks have showed SELECT 1 as slower than SELECT COUNT(*) in MyISAM, up until #91 by Damien. I know InnoDB is now our default, but we also deliberately did not put in an upgrade path for this change, so all of the hundreds of thousands of sites upgrading from 6.x and below will be stuck with MyISAM performance on these queries.

Damien, could you share why you got different results last time than anyone else? And could someone do a more holistic benchmark with ab now that this change is affecting more than just path.inc?

Sorry, I meant...

This does not need a benchmark. SELECT COUNT(*) reads from the table header and does not read from the table so of course it's faster.

Spent some time talking benchmarks with Damien in IRC. The values presented in #18, #20, and #85 are highly variable. For example, here were four more test runs @ 100 queries:

Sometimes COUNT(*) is faster on MyISAM, sometimes it isn't. There's a pretty wide margin of error.

In #91, Damien cranked the number of iterations to 100K instead of 100 to try and make these margins a bit more pronounced. Over 100K iterations, LIMIT 1 comes out on top.

The end result being that MyISAM users shouldn't see a dramatic difference, and on the whole these spots in core should be marginally faster.

Therefore, committed to HEAD. We need to document this practice in the upgrade guide, at least, and it would be great to document it in the Programming best practices guide as well.

Aside from documentation, this also needs to be backported to D6 it would seem.

Bump.

Documented in upgrade guide, moving back to D6.

Really moving back to 6 this time.

I believe it could be a good idea for module maintainers to change their count querys in D6 times, too. I'm only a bit confused why I should write SELECT COUNT(1) FROM {mytable} as written in http://drupal.org/node/224333#select_count. I like SELECT COUNT(primary_key_field) FROM {mytable} more than using anonymous numbers... nevertheless - it doesn't change so much in the result of the query, but I find it easier to read.

Is a named column slower?

on pgsql, named column is slower.

So, why this COUNT is still there in D6?

So, why this COUNT is still there in D6?

Because InnoDB isn't the default like it will be for D7.

on pgsql, named column is slower.

Why should we code differently for a broken DB server. Using named columns is beneficial for self documenting code.

I like SELECT COUNT(primary_key_field) FROM {mytable} more than using anonymous numbers

Ditto. It documents the code better but otherwise is no different. It should be just as fast (or nearly as fast) regardless of the column. It is a count of rows based on the WHERE clause if one is given. Using * you select all columns so, if you are just interested in a count of rows, using * is a bad idea. Using count(1) makes an assumption that column 1 is the primary key but that column might not be the primary key.

on pgsql, named column is slower.

Why should we code differently for a broken DB server. Using named columns is beneficial for self documenting code.

I like SELECT COUNT(primary_key_field) FROM {mytable} more than using anonymous numbers

Ditto. It documents the code better but otherwise is no different. It should be just as fast (or nearly as fast) regardless of the column. It is a count of rows based on the WHERE clause if one is given. Using * you select all columns so, if you are just interested in a count of rows, using * is a bad idea. Using count(1) makes an assumption that column 1 is the primary key but that column might not be the primary key.

COUNT(column) is a count of non-NULL values in that column. COUNT(1) is the count of rows where the constant 1 evaluates to non-NULL (that is, all rows), 1 is not column 1, but a constant.
The real thing we want to know is not the count of these rows, but whether they exist at all. It is best achieved by
SELECT EXISTS(SELECT 1 FROM {url_alias});
It's also 2 orders faster than the fastest of the cited solutions, at least on PostgreSQL. Please check it under MySQL also.

To clarify, what we are using is:

We are *not* counting.

To clarify, what we are using is:
SELECT 1 FROM {url_alias} LIMIT 1
We are *not* counting.

By "we" you must mean HEAD or something else, as D6, to which this bug is dedicated, uses COUNT...
SELECT 1...LIMIT 1 is OK from the performance point of view, it is as fast as EXISTS. Semantically, EXISTS is better.

.

COUNT(column) is a count of non-NULL values in that column.

COUNT(primary_key_field) FROM {mytable}

Primary key fields cannot be NULL.