Have you noticed that number of pages in the 'top visitors' report (admin/logs/visitors) doesn't match the real number of records that should be reported?

The queries used to build the pager look like this:

  $sql = "SELECT COUNT(a.uid) AS hits, a.uid, u.name, a.hostname, SUM(a.timer) AS total, ac.aid FROM {accesslog} a LEFT JOIN {access} ac ON ac.type = 'host' AND LOWER(a.hostname) LIKE (ac.mask) LEFT JOIN {users} u ON a.uid = u.uid GROUP BY a.hostname, a.uid, u.name, ac.aid". tablesort_sql($header);

  $sql_cnt = "SELECT COUNT(DISTINCT(uid)) FROM {accesslog}";

Note that while the GROUP BY clausule for the first query has 4 columns, the query used to compute the pager count only uses uid. In the GROUP BY clausule u.name and ac.aid depend on the uid, but hostname (the ip) does not. ;-)

The correct query to compute the pager count should look like this:

  $sql_cnt = "SELECT COUNT(DISTINCT(CONCAT(uid, hostname))) FROM {accesslog}";

Try it, and you'll see the difference.

PS: I'm sorry for not attaching a patch here, but my statistics module has several patches applied that are still pending to apply to HEAD.

CommentFileSizeAuthor
#22 stats_top_visitors_query_0.patch994 bytesnickl
#2 stats_top_visitors_query.patch691 bytesmarkus_petrux

Comments

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

not ready for review cuz it aint a real patch.

markus_petrux’s picture

markus_petrux commented
Status: Needs work » Needs review
StatusFileSize
new stats_top_visitors_query.patch691 bytes

ok, here we go.

chx’s picture

chx commented
Status: Needs review » Needs work

I do not think multiolumn DISTINCT would need CONCAT. Please look into whether select (count distinct(uid, hostname)) would work.

markus_petrux’s picture

markus_petrux commented
Status: Needs work » Needs review

I did tests. In my case, running MySQL 4.1.x, I had to use CONCAT.

dries’s picture

dries commented

Wouldn't it be better if that query did do the exact same group? Not sure which one would be faster. Can't we simplify that massive 'group by' statement. Looks like it might be painfully slow.

markus_petrux’s picture

markus_petrux commented

The main query is indeed heavy, though I don't know how it could be optimized. Maybe adding indexes? ...but that would affect performance at INSERT time (ie. at hook_exit time, for every page hit).

As far as I know, all fields specified in the GROUP BY clausule are required. That's how aggregate functions are grouped.

http://dev.mysql.com/doc/refman/5.0/en/group-by-functions.html

The count query is not so heavy. It could be optimized by adding an index by uid+hostname to the access_log table. uid+hostname is the minimum common factor of the group by clausule used in the query.

markus_petrux’s picture

markus_petrux commented

Well, the patch fixes the issue, otherwise the count query returns an incorrect result.

Performance would be a different issue...

moshe weitzman’s picture

moshe weitzman commented

if a bug fix negatively impacts performance, it won't be accepted. calling them separate issues does not help us proceed. you might be right, but it doesn't matter :)

markus_petrux’s picture

markus_petrux commented

But the performce issue is not related to the query changed by this patch. The heavy query is the one that retrieves the rows for the pager.

Have you checked with the devel module? I did, with an access table filled with more than 10000 rows. The count query is quite fast with and without the patch. The other one is slow, but it is not touched by this patch. If that should be addressed, this is IMHO a different issue.

moshe weitzman’s picture

moshe weitzman commented

ok i get it now. i got mixed up with some other proposal to add indexes to accesslog table. thanks for the clarification.

markus_petrux’s picture

markus_petrux commented

No problem. Probably my fault for not being clear when I said "Performance would be a different issue...", sorry.

dries’s picture

dries commented

Can't we come up with a different count-query? This looks like a really weird approach.

markus_petrux’s picture

markus_petrux commented

I tried to find alternatives, but none returned the expected result. I ended up with this proposal.

dries’s picture

dries commented

Can't come up with a better query either ... anyone else?

markus_petrux’s picture

markus_petrux commented

Maybe bumping caches some attention to see if anyone else can come up with a different approach?

markus_petrux’s picture

markus_petrux commented
Status: Needs review » Reviewed & tested by the community

Since no one came with a better approach, maybe the patch in #2 is RTBC?

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Needs review

I've had issues with COUNT(DISTINCT()) and PgSQL before. Needs a PgSQL check before committing.

markus_petrux’s picture

markus_petrux commented

I have opened a forum topic asking for help to review the proposed solution in #2:

http://drupal.org/node/58709

markus_petrux’s picture

markus_petrux commented

I've had issues with COUNT(DISTINCT()) and PgSQL before.

Note that the actual code uses COUNT(DISTINCT()). The proposed patch adds CONCAT(uid, hostname), so the count query returns the correct result for the table pager to compute the number of pages.

Needs a PgSQL check before committing.

Please, look at the above mentioned forum topic. grcm has been able to test the query in PostgreSQL.

I can't think of any other alternative.

moshe weitzman’s picture

moshe weitzman commented

FYI, we had a similar CONCAT() in our main node_access query and it performed very poorly with large data sets because it could not use some index. FYI.

markus_petrux’s picture

markus_petrux commented

I think the only way to improve performance for this query would be to add an index for uid+hostname.

This is just used in admin/logs/visitors. Not sure how important the performance hit is, overall. The number of records to keep on the accesslog table depends on site traffic, but also on the number of days option of the module.

OTOH, node_access is used in the user side. ie. performance was much important. It is not comparable, I think.

nickl’s picture

nickl commented
StatusFileSize
new stats_top_visitors_query_0.patch994 bytes

Rerolled to head.
The issue refers to:

...number of pages in the 'top visitors' report (admin/logs/visitors)...

I don't have number of pages I only have: Hits, Visitor, Total page generation time and Operations
Results are the same with or without the patch.

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: x.y.z » 5.x-dev

This seems to double the time it takes to execute the query (tried on Drupal.org's database server). However, it is only for one admin page. I'm not sure adding keys and such would be a good idea since we need to keep inserts fast as our priority.

Other reviews?

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Fixed

Committed to HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)