Watchdog calls same columns twice [#105540]

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.

Comment	File	Size	Author
#3	watchdog_sql_cleanup_1.patch	1.03 KB	Souvent22
#3
#2	watchdog_sql_cleanup_0.patch	856 bytes	Souvent22
#2
	watchdog_sql_cleanup.patch	849 bytes	Souvent22

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

moshe weitzman CreditAttribution: moshe weitzman commented 28 December 2006 at 04:27

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

Comment #2

Souvent22 CreditAttribution: Souvent22 commented 28 December 2006 at 04:36

File	Size
watchdog_sql_cleanup_0.patch	856 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;

Comment #3

Souvent22 CreditAttribution: Souvent22 commented 28 December 2006 at 04:46

File	Size
watchdog_sql_cleanup_1.patch	1.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.

Comment #4

moshe weitzman CreditAttribution: moshe weitzman commented 28 December 2006 at 04:48

Status:

Needs review

» Needs work

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

Comment #5

m3avrck CreditAttribution: m3avrck commented 28 December 2006 at 23:25

Status:

Needs work

» Reviewed & tested by the community

Seems Souvent22 and Moshe cross posted each other.

Comment #6

m3avrck CreditAttribution: m3avrck commented 28 December 2006 at 23:27

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.