I don't have the time to check this further, sorry, so I'm only reporting here a _possible_ bug.

We have the following code in the 'dblog' module:

/**
 * Implementation of hook_cron().
 *
 * Remove expired log messages and flood control events.
 */
function dblog_cron() {
  // Cleanup the watchdog table
  $min = db_result(db_query('SELECT MIN(wid) FROM {watchdog}'));
  if ($min) {
    $max = db_result(db_query('SELECT MAX(wid) FROM {watchdog}'));
    if ($max) {
      if (($max - $min) > variable_get('dblog_row_limit', 1000)) {
        db_query('DELETE FROM {watchdog} WHERE wid < %d', $max - $min);
      }
    }
  }
}

But the "DELETE FROM ,,," arithmetic looks suspicious. Shouldn't it be as follows?

db_query('DELETE FROM {watchdog} WHERE wid < %d', $max - variable_get('dblog_row_limit', 1000));
CommentFileSizeAuthor
#1 dblog_cron_cleanup-178044-1.patch772 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
dblog_cron_cleanup-178044-1.patch772 bytes

So, let's say we want to keep 1000 rows. If the min is 10000 and the max is 12000 then this line will attempt to delete wids below 2000. No cigar. With your suggested change we will try to delete entries below 11000. Sounds just right.

But there is more. Let's see this code snippet:

      if ($max - variable_get('dblog_row_limit', 1000) > $min) {
        db_query('DELETE FROM {watchdog} WHERE wid < %d', $max - variable_get('dblog_row_limit', 1000));
      }

Now, what happens if you run the DELETE when the if fails? It means that the limit we are deleting from is smaller than the smallest of wids -- therefore zero line is deleted. So, the if can be happily removed. And to further our cleanup, it's totally okay if $min and $max are 0 because then we try to delete from an empty table... Not much remains :)

mooffie’s picture

mooffie CreditAttribution: mooffie commented

Your code is mathematically correct. I hope to actually test it later today.

And,

Do you see any value in deleting a bit more records than needed? For example, 50 extra records:

$padding = 50;
db_query('DELETE FROM {watchdog} WHERE wid < MAX(wid) - %d + %d',
       variable_get('dblog_row_limit', 1000), $padding);

Because deleting the exact number, as your code does, has the disadvantage that we will delete record(s) on every cron run.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

chx is right. The proposed patch looks better. Good catch guys. Committed to CVS HEAD.

Anonymous’s picture

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