Watchdog has 2 instances of the uid column when using this query:

SELECT w.*, u.uid FROM {watchdog} w INNER JOIN {users} u ON w.uid = u.uid;

Patch updates this query to:

SELECT w.severity, w.type, w.timestamp, w.message, w.link, u.name FROM {watchdog} w INNER JOIN {users} u ON w.uid = u.uid

The columns are enumerated because its generally good practice in high performance SQL coding to enumerate your columns (no matter how many) so the database doesn't have to do a call to its system table to review all the columns in the tables and their types and act accordingly. By enumerating, it grabs the column by index (hash prob.) from the system table which contains the column name, type.

CommentFileSizeAuthor
#3 watchdog_sql_cleanup_1.patch1.03 KBSouvent22
#2 watchdog_sql_cleanup_0.patch856 bytesSouvent22
watchdog_sql_cleanup.patch849 bytesSouvent22

Comments

moshe weitzman’s picture

moshe weitzman commented

the nwe query omits *both* uid columns. is that intended. if so, this is a reasonable change.

Souvent22’s picture

Souvent22 commented
StatusFileSize
new watchdog_sql_cleanup_0.patch856 bytes

Thanks. You're right, it does omit both..as it should..however, I DID forget the wid column. Re-rolled patch.

I looked through the code ( which is quoted below for easy review), and uid is not used anywhere.

   // ............ general code....then....
  $sql = "SELECT w.wid, w.severity, w.type, w.timestamp, w.message, w.link, u.name FROM {watchdog} w INNER JOIN {users} u ON w.uid = u.uid";
  $tablesort = tablesort_sql($header);
  $type = $_SESSION['watchdog_overview_filter'];
  if ($type != 'all') {
    $result = pager_query($sql ." WHERE w.type = '%s'". $tablesort, 50, 0, NULL, $type);
  }
  else {
    $result = pager_query($sql . $tablesort, 50);
  }

  while ($watchdog = db_fetch_object($result)) {
    $rows[] = array('data' =>
      array(
        // Cells
        $icons[$watchdog->severity],
        t($watchdog->type),
        format_date($watchdog->timestamp, 'small'),
        l(truncate_utf8($watchdog->message, 56, TRUE, TRUE), 'admin/logs/event/'. $watchdog->wid, array(), NULL, NULL, FALSE, TRUE),
        theme('username', $watchdog),
        $watchdog->link,
      ),
      // Attributes for tr
      'class' => "watchdog-". preg_replace('/[^a-z]/i', '-', $watchdog->type) .' '. $classes[$watchdog->severity]
    );
  }

  if (!$rows) {
    $rows[] = array(array('data' => t('No log messages available.'), 'colspan' => 6));
  }

  $output .= theme('table', $header, $rows);
  $output .= theme('pager', NULL, 50, 0);

  return $output;
Souvent22’s picture

Souvent22 commented
StatusFileSize
new watchdog_sql_cleanup_1.patch1.03 KB

Just reviewed and tested the patch. uid is used in the theme('username', $obj) function. w/o uid, it will still display the username, but withe a (not verified) clause. Fixed with this patch. that should be it.

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

uid is needed by this line: theme('username', $watchdog). after that, looks RTBC to me.

m3avrck’s picture

m3avrck commented
Status: Needs work » Reviewed & tested by the community

Seems Souvent22 and Moshe cross posted each other.

m3avrck’s picture

m3avrck commented
Version: 5.0-rc1 » 5.x-dev

Also as a note, we should look into carrying this practice throughout core (more small patches like this that are easy to digest) and updating the coding guidelines. Seems like a nice and easy small performance gain.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

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