inconsistent "who's online" message

Here is a very simple patch. The number of online users is now quantified using db_num_rows() on the list of online (authenticated) users.

Attachment	Size
online_block.patch	805 bytes

This patch also corrects the time_period, currently users are never removed from the list. The query checks for access time >= period (period = 900 by default) instead of access time >= time() - period.

Attachment	Size
online_block_0.patch	984 bytes

Works as expected. This also removes a query.

For those reviewing the patch and confused about the time() bug mentioned the time_period assignment helps explain it:

Since we are already subtracting from time the query was asking for users who's last access was >= time() - time() - 900. That's clearly not correct :). Good catch profix898, everything looks in order.

But isn't the implementation of sess_count() wrong than? (We use that function in a couple of places.)

The rows in session table are removed the moment the users logs off. But in users table the access timestamp remains. We are running a query on sessions table to get the number of 'active' sessions, but for the user list we receive all entries from the users table where last access is within the time_period interval. What means in 4.7 and 5.0b1 users which logged out some time ago (within the intervall) are still listed, although their sessions entry was already removed.
IMO its is more consistent to remove the user from "Who's online" block the moment he/she logs out. This also removes the inconsistency between the online block and sess_count.

This patch looks up the uids of all (non-anonymous) sessions from sessions table (same as sess_count) and uses INNER JOIN to get the according usernames (for the user list). The number of users is determined using db_num_rows() on the result (to save an additional query, as in my revious patch), but this time the result is consistent with sess_count($time_period, false).

Attachment	Size
online_block_1.patch	1.02 KB

Ah, yeah my logged out user was still showing up. The JOIN fixes it. This was probably the root of the demonstrated problem in the first place.

Sorry for the confusion about sess_count, it is implemented correctly. It expects a timestamp for its first argument which is perfectly reasonable. $time_period in the block function is just a horrible name for the variable. At one point it was probably storing the time period from variable_get but now it is storing a timestamp derived from that period. The old authenticated user query was never changed to reflected this and that was the problem.

Tested with second user logging in and then logging out. Users and counts match and display correctly.

Same patch as above, but with time_period renamed to timestamp. Choose what you like better.

Attachment	Size
online_block_2.patch	1.35 KB

Actually, elsewhere in the code we write $interval. ;-)

Whatever you like ... but in sess_count() it is called$timestamp already and even the db field is named 'timestamp'.
Again same patch as above, but with variable renamed to $interval :)

Attachment	Size
online_block_3.patch	1.35 KB

Online_block_3.patch applyed and result OK.
but I noticed a longer Frontpage Recall-time !!
Reference Site: http://www.adaccs.at/test

Has anyone looked at the performance implications? My brief review shows a filesort was added.

Its a little difficult to benchmark this, because performance depends heavily on the number of users and sessions in the tables, but actually my install is even slightly faster with the patch.

I took a clean install of Drupal 5.0b1, created 500 users and 50 sessions, enabled the block and set FF2 to refresh every 5s (with all caching disabled incl. query cache, of course). After 250 refreshes I took a look a the query log ...

before (without patch):
sess_count() (500x): 7.41 ms (executed 2x per refresh, anonymous + registered users)
user_block() (250x): 9.81 ms
=> 2 x 7.41 ms + 9.81 ms = 24.63 ms

after (with patch):
sess_count() (250x): 7.43 ms (executed only 1x per refresh, anonymous users)
user_block() (250x): 12.98 ms
=> 7.43 ms + 12.98 ms = 20.41 ms

Although user_block() is slower with the JOIN query, the block is still faster than before (with the 2x sess_count()). I dont know how significant these results are!? Maybe someone can verify?

@drumm: What do you mean 'filesort was added'? If you are talking about the 'ORDER BY', it was already there.

Yeah, the filesort and temporary table are showing up because of the ORDER BY. I found the query doesn't use the temporary table when I used ORDER BY s.timestamp DESC. I think this provides the same functionality and should be a more effecient query.

Right, using s.timestamp instead of u.access is better. All query parameters (WHERE, ORDER BY) are against the 'sessions' table then. Patch updated accordingly. I will repeat the benchmark with the new query as time permits.

Attachment	Size
online_block_4.patch	1.35 KB

The query with s.timestamp is about 2x faster (12 ms => 6 ms). Resulting in a total query time of 13.87 ms (normalized to values reported in #13).

(New patch only removes a redundant blank line from patch in #15)

Attachment	Size
online_block_5.patch	1.43 KB

So we fix the bad behavior and have less time used by queries. Sounds like a win/win.

Patch works for me.

Committed to CVS HEAD. Thanks.