dblog_cron() deletes too many records when auto_increment > 1

To get around the issue of auto_increment value greater than 1 , the queries could be changed to do the following to leave the most recent 1000 records:

Instead of selecting the max(wid) and subtracting 1000
1) select the min(wid) from the most recent 1000 records
2) delete everything less than the resulting min(wid).

SELECT min( wid )
FROM (

SELECT wid
FROM `watchdog`
ORDER BY wid DESC
LIMIT 1000
) AS t1

If this solution addresses your issue correctly, I would be happy to have the bug assigned to me to fix.

once this lands for D7 ... this will need to be ported to D6 (where I can confirm the problem)

Great, I'll start working on a patch.

I'll likely end up using db_query_range() or other parts of the database abstraction layer, because not all databases agree on the format of the LIMIT syntax.

Yep, looking at it now.

Here's a solution using dynamic queries from the D7 database APIs, which should avoid vendor specific issues. The code looks good so far and I'll roll a patch after further testing.

function dblog_cron() {
  // Cleanup the watchdog table

  // Get the row limit set for the watchdog table (default 1000)
  $rowlimit = variable_get('dblog_row_limit', 1000);
 
  // If row limit is n, get the wid of the nth row if it exists, ordering by the newest entry first.
  // Counting the most recent n rows avoids possible issues with wid number sequences,
  // e.g. auto_increment value > 1 or rows have been deleted directly from the table. 
  if ($rowlimit > 0) {
    $result = db_select('watchdog', 'w')
      ->fields('w', array('wid'))
      ->orderBy('wid', 'DESC')
      ->range($rowlimit-1, 1)
      ->execute(); 

    //Delete all table entries older than the nth row.  Only executes if nth row was found. 
    foreach($result as $min_row) {
      $num_deleted = db_delete('watchdog')
        ->condition('wid', $min_row->wid, '<')
        ->execute();
    }
  }
}

Here's the patch, with some additional edits made to the code in my last post regarding coding guidelines and a check for a successful query execution.

I would be happy to help with the D6 port as needed, and have drafted code for that.

Made updates as recommended in this new patch revision, now using fetchField(), and not using $num_deleted variable.

I think this needs a test too

Thanks for the very helpful and speedy feedback. Here's a summary of the tests I have run on the patches submitted:

Running the cron task when:
1) No rows in the watchdog table
2) Less rows than the limit in the watchdog table
3) More rows than the limit in the watchdog table
4) Same number of rows as the limit in the watchdog table
5) Tests 1-4 with the increment value changed to greater than 1
6) Tests 1-4 with the increment value returned to 1
7) Tests 2-4 with rows deleted directly from the table out of wid sequence order (e.g. not oldest or newest rows)
8) DBQuery fails (e.g. returned result is FALSE)

Does this cover the testing scenarios you had in mind?
If so, I'll go ahead and re-roll the patch without the errant white space.
If not, could you give me a pointer or more info on the associated test you mentioned?

hbergin: Those are exactly the test scenarios we should be covering, but we should write some simple tests (http://drupal.org/simpletest) so that this error doesn't resurface. I'm not 100% certain we can write a test that checks how the database performs with an autoimcrement of >1, but we should look into it at the very least

Here is the patch without the errant white space, as there's no need to hold off on that fix. Simple test is a new area for me, but I'm happy to help investigate as I agree there's great value in automated test suites.

Great, I was reading that general issue thread and was thinking the same thing.

I see the dblog.test file verifyCron() function already includes the test for rows exceeding the limit in the watchdog table, so I could look to complement this test scenario with:

Running the cron task when:
1) No rows in the watchdog table
2) Less rows than the limit in the watchdog table
3) More rows than the limit in the watchdog table (already part of simple test)
4) Same number of rows as the limit in the watchdog table
5) Tests 2-4 with rows deleted directly from the table out of wid sequence order (e.g. not oldest or newest rows)

I agree the code already in dblog.test works just fine using COUNT(wid).

My thought was that the verifyCron() function only includes one test scenario at the moment, and we could add additional scenarios from the ones listed in #19.

Checked out the HEAD code today. Here are patches rolled against this latest source code. Both were tested using the D7 development testing feature and run through the coder module.

dblog-cron-watchdogcleanup-917134.patch
Same code changes as in #17, but against latest source code.

dblog-cron-watchdogcleanup-tests-917134.patch
As per #21, this patch adds test variations to dblog.test verifyCron() function, using similar format to the already existing test.

Test were added to check the resulting rows in the watchdog table after running the cron task when:
1) No rows in the watchdog table
2) Less rows than the limit in the watchdog table
3) Same number of rows as the limit in the watchdog table
4) More rows than the limit in the watchdog table, where rows are out of wid sequence order (e.g. due to ad hoc direct deletions from the table)

+++ modules/dblog/dblog.module	1 Oct 2010 20:31:24 -0000
@@ -100,12 +100,26 @@ function dblog_init() {
+  // Get the row limit set for the watchdog table (default 1000).

This code comment is redundant.

dblog-cron-watchdogcleanup-917134.patch
Redundant comment removed as per #23.

dblog-cron-watchdogcleanup-tests-917134.patch
Comment edits.

#24: dblog-cron-watchdogcleanup-tests-917134.patch queued for re-testing.

#25 had 2 failures in database_test.test related to function testAlterRemoveRange().
#27 had 1 failure in upgrade.test related to function public function testBasicUpgrade()

Could not spot any direct correlation with patch to dblog.test verifyCron() changes which only accesses the watchdog table and runs cron. The fails in #25 and #27 were in different places, with the same patch code.

Requeuing once more.

#24: dblog-cron-watchdogcleanup-tests-917134.patch queued for re-testing.

D6 equivalent syntax for dblog_cron() solution backport. Tested against D6 install.

function dblog_cron() {
  $row_limit = variable_get('dblog_row_limit', 1000);
  if ($row_limit > 0) {
    $min_row = db_result(db_query_range('SELECT wid FROM {watchdog} ORDER BY wid DESC', ($row_limit-1), 1));
    if ($min_row) {
      db_query('DELETE FROM {watchdog} WHERE wid < %d', $min_row);
    }
  }
}

Committed to CVS HEAD. Thanks!

This patch should have follow-up to use Database::getConnection('default', 'default') except direct db_select()
last commit kills ability to use master/slave feature for dblog
also db_select is bad here because it's alterable

andypost ... i would open a new issue for that and just reference this one for more info

this issue should stay open for the backport however

Similar tests were run on this d6 backport patch as listed in #15. Also run through coder module.

Marked #551688: Row Limit Doesn't Take auto_increment_offset into account as a duplicate.

@bleen18 I think no reason in different issue because this still needs work

@andypost, does this revised syntax address your concerns? If so, I can look at re-rolling a patch with this syntax.

When I first wrote the patch, some api documentation indicated that db_query_range was going to be deprecated, but I do not see that in the current documentation, so am assuming it's OK to use it now.

$row_limit = variable_get('dblog_row_limit', 1000);

if ($row_limit > 0) {
  $query = 'SELECT wid FROM {watchdog} ORDER BY wid DESC';
  $min_row = db_query_range($query, $row_limit-1, 1)->fetchField();

  if ($min_row) {
    Database::getConnection('default', 'default')->delete('watchdog')
      ->condition('wid', $min_row, '<')
      ->execute();
  }
}

Patch updated to use syntax in #39 to address #32.
Run on local D7 HEAD install using simpletest module and coder module.

As per #21, this patch adds test variations to dblog.test verifyCron() function, using similar format to the already existing test.

Test were added to check the resulting rows in the watchdog table after running the cron task when:
1) No rows in the watchdog table
2) Less rows than the limit in the watchdog table
3) Same number of rows as the limit in the watchdog table
4) More rows than the limit in the watchdog table, where rows are out of wid sequence order (e.g. due to ad hoc direct deletions from the table).

@hbergin, please merge #40 and #42 in one patch, I think it's RTBC if both in one

Thanks for the feedback, @andypost. Here is a patch which merges #40 and #42, as requested in #43.

Patch no longer applies.

Relloled.