CGI.pl's confirm_login() function runs a query to delete old login cookies whenever a script runs that function and the user is not logged in (i.e. quietly_check_login does not return a true value). The query takes a number of seconds to complete (I saw these queries running for 15-26 seconds on b.m.o.) and causes quietly_check_login()'s user authentication queries to lock waiting for it to complete, which slows down overall Bugzilla performance. The login cookies deletion query should not run on such a regular basis.

On b.m.o. I added this command to the crontab file and deleted the corresponding code from CGI.pl: # 2002-03-28 myk@mozilla.org bug134022: # Delete old login cookies once per day instead of every few minutes # (as the Bugzilla code in CGI.pl was doing until I commented it out). # This query takes a non-negligible time to run (up to 30 seconds) # and locks the logincookies table while it runs, blocking all login # checks in the meantime. Running this query once a day is sufficient # and should improve Bugzilla performance. 0 5 * * * /opt/mysql/bin/mysql --execute='DELETE FROM logincookies WHERE TO_DAYS(NOW()) - TO_DAYS(lastused) > 30' bugs

How many login cookies are in the table? What does: SELECT vount(cookie) FROM logincookies, profiles WHERE logincookies.cryptpassword != profiles.cryptpassword AND logincookies.userid = profiles.userid return? Remember that the version of bugzilla bmo runs almost never deletes login cookies from the db, so there is probably a lot of unused entries in there. People behind rotating proxies will build up a new set every time their proxy changes, too. What % of the cookies in bmo have not been used in the past week, say?

Actually, better idea. The query as-is doesn't use an index, probably because lastused is wrapped in a function. How long does: SELECT userid FROM logincookies WHERE lastused > date_sub(NOW(), interval 30 day); take? EXPLAIN shows that version using an index, at least, although I have a small table, so it may not be much better.

>Remember that the version of bugzilla bmo runs almost never deletes login >cookies from the db Actually, until yesterday, it deleted login cookies from the database every few minutes. The latter query took 6.48 seconds recently (but times probably vary with load). Keep in mind that the problem is not just the amount of time the query takes but that it blocks other queries while it executes. Even six seconds of blockage is significant if it takes place every few minutes, and it isn't necessary. If we are only deleting login cookies older than 30 days, we don't have to run the query more than once a day, because it doesn't matter if we delete a cookie at 30.1 instead of 30.9 days.

I meant that it never deletes it except for there - logouts didn't, for example. I just don't like having to run stuff via cron, I guess. What about bundling this into the once-per-hour versioncache rebuild? You didn't mention how many logincookies there were, or what percentage of them were over a week old.

Bumping priority/severity. Mozilla 1.0 will be out soon, so anything having to do with performance is going to be a high priority in the short term. Mothra is straining already.

Using the versioncache timetable is a big improvement, but it's still too often, and I don't see why we're rebuilding versioncache regularly anyway. Shouldn't we rebuild it only when the installation configuration changes or manually when it's messed up in some way? Anyway, what's the problem with cron? It's the right tool for the job unless we want to re-invent the wheel. It could be more user-friendly, of course; is there a way for checksetup.pl to add cron jobs?

cron is really system specific, not to mention that some users may not be allowed to use cron. I really don't think that 6 seconds per hour is that much of a big deal. Personally, I want to remove versioncache entirely, eventually. Moving stuff like versions to a custom field will make most of it obsolete, and syncronisation once we start having mod_perl will be fun (we already have possible race conditions with this, probably)

cron is system-dependent, but schedulers aren't. I'm loathe to re-invent cron just for Bugzilla.

True, but remember that windows doesn't have cron, and I'm not sure how flexable its |at| daemon is. We could probably remove the call to clear the token table then, too.

>True, but remember that windows doesn't have cron, and I'm not sure how >flexable its |at| daemon is. Let's just say that at is sufficiently flexible for most of the uses any sensible admin might want. People who run Bugzillas on Win32 must use at to run collectstats anyway; making them run one more thing doesn't make such a big difference. But we do need documentation for this. But it's totally another issue whether we should avoid doing cron jobs. Or if we do them, should we aim at having only one cron job which would do all the things (collectstats and so on; I don't believe we're going to see a day when no automated daily maint tasks are necessary)? Personally, I don't have a problem with cron/at. They are easy to configure, at least when aided by the documentation we're going to supply (:-E). Also, they are a part of standard systems maintenance anyway, especially on Unix platforms.

Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being retargeted to 2.20 If you plan to act on one immediately, go ahead and pull it back to 2.18.

Myk, Dave: is this bug still an issue today?

dunno. b.m.o still has that line commented out, and has it running on a cron job :)

(Should be assigned to patch author, even though the patch is really old.) This should be a fairly simple thing to do. Let's just move it into the versioncache rebuild. That will solve the actual noticeable performance problem, which is the bug here.

I think we could do this in 2.20.1 or something like that.

Umm.... Why not add the userid to the WHERE clause so that it delete's only that user's old cookies during login? That will use an index and should be very quick.

(In reply to comment #18) > Why not add the userid to the WHERE clause so that it delete's only that user's > old cookies during login? That will use an index and should be very quick. You would miss all users who logged in only once and never came back. If we don't care too much about how often old login cookies are deleted, we could as well move this part in checksetup.pl.

This bug is the last one in the Bugzilla product with a severity of "blocker" or "critical" which isn't fixed yet. This makes me think that either this bug isn't so critical, or that it should be fixed asap. By looking at its ID, I think it isn't a critical bug. Decreasing the severity to major.

Only security and dataloss fixes will be accepted on the 2.20 branch.

We are freezing the code for 3.0 in two weeks and we don't expect this bug to be fixed on time.

This makes the logincookie deletion use an index. It also makes it only happen after a successful login with some login method that actually requires cookies.

Comment on attachment 356143 [details] [diff] [review] v2 >+ . $dbh->sql_to_days('LOCALTIMESTAMP(0)') . " - " Why do you replace NOW() by LOCALTIMESTSAMP(0)? We did it in BugMail.pm in bug 305968 because NOW() could be off by one second. I think this doesn't matter here.

(In reply to comment #24) > Why do you replace NOW() by LOCALTIMESTSAMP(0)? We did it in BugMail.pm in bug > 305968 because NOW() could be off by one second. I think this doesn't matter > here. I'd like to standardize on LOCALTIMESTAMP(0), particularly because LOCALTIMESTAMP is ANSI SQL and NOW() is not.

Comment on attachment 356143 [details] [diff] [review] v2 >+ elsif ($fail_code == AUTH_LOGINFAILED or $fail_code == AUTH_NO_SUCH_USER) { >+ $dbh->do("DELETE FROM logincookies WHERE lastused < " (In reply to comment #23) > This makes the logincookie deletion use an index. It also makes it only happen > after a successful login with some login method that actually requires cookies. I don't see it as successful at all. Here, the login failed.

Comment on attachment 356143 [details] [diff] [review] v2 Wow, weird, this is an old version of the patch. I'll attach the right one.

Okay, here's the right version.

Comment on attachment 356658 [details] [diff] [review] v3 >Index: Bugzilla/Auth.pm >- elsif (($fail_code == AUTH_LOGINFAILED) || ($fail_code == AUTH_NO_SUCH_USER)) { >+ elsif ($fail_code == AUTH_LOGINFAILED or $fail_code == AUTH_NO_SUCH_USER) { There is no reason to do this change. == has higher precedence than ||, so replacing it by "or" has no justification here. Also, this is completely unrelated to this bug (and Bonsai would then point to this bug instead of the original bug when looking at the history of this file). Please revert that. >Index: Bugzilla/Auth/Persist/Cookie.pm >+ # Issuing a new cookie is a good time to clean up the old >+ # cookies. Hum... Why not doing this on logout, while we clear the login cookie of the user? >+ $dbh->do("DELETE FROM logincookies WHERE lastused < " >+ . $dbh->sql_to_days('LOCALTIMESTAMP(0)') . " - " >+ . $dbh->sql_interval(MAX_LOGINCOOKIE_AGE, 'DAY')); PostgreSQL crashes (sorry, the error message is in french): DBD::Pg::db do failed: ERREUR: l'opérateur n'existe pas : integer - interval LINE 1: ...used < TO_CHAR(LOCALTIMESTAMP(0)::date, 'J')::int - 30 * INT... ^ HINT: Aucun opérateur ne correspond au nom donné et aux types d'arguments. Vous devez ajouter des conversions explicites de type. at Bugzilla/Auth/Persist/Cookie.pm line 74 Bugzilla::Auth::Persist::Cookie::persist_login('Bugzilla::Auth::Persist::Cookie=HASH(0x9515640)', 'Bugzilla::User=HASH(0x96130f0)') called at Bugzilla/Auth.pm line 147 Bugzilla::Auth::_handle_login_result('Bugzilla::Auth=HASH(0x94f8378)', 'HASH(0x8d6ff78)', 2) called at Bugzilla/Auth.pm line 92 Bugzilla::Auth::login('Bugzilla::Auth=HASH(0x94f8378)', 2) called at Bugzilla.pm line 241 Bugzilla::login('Bugzilla', 0) called at /var/www/html/bugzilla-pg/index.cgi line 40

(In reply to comment #29) > Hum... Why not doing this on logout, while we clear the login cookie of the > user? Because most users never log out. > PostgreSQL crashes (sorry, the error message is in french): What version of Pg are you using?

You should never depend on people knowing the exact precedence order. Anyhow, I fixed the actual bug.

(In reply to comment #30) > Because most users never log out. But there is still at least one user who logs out at least once per month. There is no need to have this code executed every hour. Anyway, I have no problem with keeping this SQL call here.

Comment on attachment 357030 [details] [diff] [review] v4 r=LpSolit assuming you made sure the table index is correctly used here. Also, I still think your || -> or change has nothing to do here (and no other test condition uses "or" in our Bugzilla code, so you introduce inconsistency).

tip: Checking in Bugzilla/Auth.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth.pm,v <-- Auth.pm new revision: 1.21; previous revision: 1.20 done Checking in Bugzilla/Constants.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v <-- Constants.pm new revision: 1.104; previous revision: 1.103 done Checking in Bugzilla/Auth/Persist/Cookie.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v <-- Cookie.pm new revision: 1.8; previous revision: 1.7 done 3.2: Checking in Bugzilla/Auth.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth.pm,v <-- Auth.pm new revision: 1.20.4.1; previous revision: 1.20 done Checking in Bugzilla/Constants.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v <-- Constants.pm new revision: 1.92.2.9; previous revision: 1.92.2.8 done Checking in Bugzilla/Auth/Persist/Cookie.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v <-- Cookie.pm new revision: 1.5.4.3; previous revision: 1.5.4.2 done