Introduce an autoincrement integer id [#229797]

There is big performance problem with big amount of tickets.
To improve this I made small fix:

In loginticket_delete function:
before:

  foreach ($tickets as $ticket) {
    $conds[] = "passcode_md5 = '%s'";
    $params[] = $ticket->passcode_md5;
    loginticket_invoke_all('delete', $ticket);
  }

  // delete all the processed tickets
  db_query("DELETE FROM {loginticket} WHERE ". implode(" OR ", $conds), $params);

now:

  foreach ($tickets as $ticket) {
    $conds[] = '"%s"';
    $params[] = $ticket->passcode_md5;
    loginticket_invoke_all('delete', $ticket);
  }

  // delete all the processed tickets
  if (count($conds)) {
  	db_query('DELETE FROM {loginticket} WHERE passcode_md5 IN ('. implode(', ', $conds) . ')', $params);
  }

And also one minor modification to loginticket_check_expiration function:
before:

  $time = getdate();
  $expire = mktime($time['hours'], $time['minutes'], $time['seconds'],
                   $time['mon'] - 2, $time['mday'], $time['year'], 0
  );

now:

  $expire = strtotime('-2 months');

Comment	File	Size	Author
#7	loginticket.install_1.patch	1.91 KB	benkewell
#7	loginticket.module_1.patch	1.99 KB	benkewell
#5	loginticket.install.patch	1.88 KB	benkewell
#5	loginticket.module.patch	1.95 KB	benkewell

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

vgarvardt CreditAttribution: vgarvardt commented 4 March 2008 at 13:20

And also, it would be much better and faster to have integer autoincremet value as primary key for data manipulation (DELETE by int ID but not by varchar string) and passcode_md5 as unique key.

Comment #2

jpetso CreditAttribution: jpetso commented 4 March 2008 at 18:56

Thanks for the suggestions, looks good mostly. Note that I can't put the %s placeholder in double quotes as that's not compatible with Postgres, but other than that, these are very sensible suggestions. (I especially like the month trick, didn't know I could do that.) Let's see if I can push out a quick point release including those.

As for the unique key... yeah, I had already considered this as well. Seems like it's a better idea than the password hash string. The question for me is now whether to include this change in a 5.x-1.x release, or roll a 5.x-2.0 for this purpose (it's an API change after all), or just wait until the Drupal 6 port which isn't far away anyways.

So, thanks for the feedback and for using this module in the first place :)

Comment #3

jpetso CreditAttribution: jpetso commented 4 March 2008 at 19:59

Status:

Needs review

» Active

Ok, I committed in your suggestions from the original post. I had wanted to roll a bug-fix release anyways, so in a few minutes you should be able to download 5.x-1.5 which includes your improvements :)

I left out the primary key issue, that one needs a bit more work and testing, no time for that right now. (Also, I want to have a second additional column 'has_expired' in order to create a hook_loginticket($op='expired') hook. Those two table changes should probably go into the same update.)

Comment #4

jpetso CreditAttribution: jpetso commented 30 May 2008 at 22:19

Title:	Performance problem	» Introduce an autoincrement integer id
Version:	5.x-1.4	» 5.x-1.5

Renaming the issue title to reflect the current need.

Anybody who wants to work on this task, by chance? Because I'm not sure when I can get back to the loginticket/temporary_invitation combo - I also have work to do on a good number of other projects and my company has no priorities for a Drupal 6 port of this at the moment (which would make me implement this issue as a prerequisite).

Comment #5

benkewell CreditAttribution: benkewell commented 22 March 2009 at 12:42

File	Size
loginticket.module.patch	1.95 KB
loginticket.install.patch	1.88 KB

i attached 2 patches which implement the primary key issue on the 5.x-1.5 code
now there is a 'tid' field which is the primary key storing an auto-increment integer id
and the delete function is using 'tid' instead of 'passcode_md5' now
and there is a db table update included for updating from existing 5.x-1.5 table

the 'has_expired' issue isn't included
as i'm not sure what the hook and the column are about
and i have no experience in writing hook too

these patches are working fine on my development website
please help test them out

Comment #6

jpetso CreditAttribution: jpetso commented 24 March 2009 at 08:37

Status:

Active

» Needs review

Need to test if Postgres handles auto_increment the same way, and $placeholders[] = "'%d'"; should not be quoted in single quotes as it's an integer. Other than that, this looks mighty fine. In order to actually improve performance, there might be the one or the other place where fetching by passcode_md5 can be replaced by fetching by tid.

Now that I think of it... tid is a bit overloaded (hi term id from core!), so let's maybe use ticket_id. Oh, and sorry about the late reply... if I keep not committing cool patches in time, I might just hand over commit access to you. But not yet.