very slow query

so this query was removing events from the event table that were past there time and thus no need to save the event. The problem with this query is it sub-queries the queue table which is larger. The query would benefit from a DISTINCT(eid) on the sub query but that still isn't desirable.

The attach patch operates on the following principle
remove old events that where created at such a time ago that there couldn't be anything in the queue.

Basically, creation time < time() - max_interval then delete that event. Haven't tested this patch throughly but I believe, at least in spirit, it is the best way to go.

another way would be to start select 200 eids from the event order by creation ASC. then ask queue if it has any thing for that eid, if it does allow the even to live on. This way this query is controllable and can't run rampant.

Yes, you catched some bad query.

The proposed solution looks good in principle but I'm afraid there may be some race conditions that make it fail. One of them is when you have a lot of processing to be done, that timeout may not be reliable. Also if you have only an interval, and it is inmediate (=0), this may delete all queries.

Here's some tentative solution I've already committed into the module, let me know whether it works. If so it is to be ported to 6.x too.

function notifications_process_prepare($module = 'notifications') {
  // Clean up event table. As events are created sequentially, we use this fact to speed up the query
  // This expiretime will prevent some race condition that occurs when the event is saved but the subs queue not yet populated  
  $expiretime = time() - 60;  
  db_query("DELETE FROM {notifications_event} WHERE created < %d AND eid < (SELECT MIN(eid) FROM {notifications_queue})", $expiretime);
....
}

About that race condition, well, I'm not sure that's the word, but the issue is like this:

With the first query we were deleting all the events queued before [max time interval] + some safety margin (which was 60 secs but could be any). That will work if all pending notifications are sent out on each cron run, but because of time constraints, depending on server load, that may not be always the case, it is possible that you have a huge number of queued notifications for an event that take a few cron runs to complete, then you'd be losing these ones.

Worst case scenario: you have only an interval that is inmediate sending, time = 0. Then [max time interval] + [60 secs] = 60 secs. If you run cron every 5 minutes and drop all the events queued more than 60 seconds ago, then you'll be deleting most of them before sending out the notifications...

Whatever, thanks for pointing out these bad queries, it's really helpful to have real performance data from huge sites.

Ported to 6.x

Will this be ported to the D5 version?

It was already in 5.x, please read the thread

Of course, sorry about that.