Instead of calling 2 functions per row, and only checking for stuff changed in the last two days, rescanall should use the lastdiffed column. However, it turns out that edit*.cgi doesn't keep the auto-updating delta_ts field the same. As a result, this will (for one run) show false positives. It won't actually send mail, but it probably will take some time. Ideally, this should be run nightly, to catch email changes which don't get picked up, eg by people pressing stop on their browser too early. I sonsidered adding this to a sanity check, but since: a) this fixes itself on the next comment, b) getting these emails nightly could help catch bugzilla bugs, and c) this will happen in real situations, I didn't think I should. myk, how long does: "SELECT bug_id from bugs where lastdiffed < delta_ts" take on b.m.o? I'd guess much less that the current stuff, but it may take too long to run nightly. patch coming.

The query takes nine seconds to run on b.m.o. and retrieves 10,199 records.

hmm. Myk, after this gets checked in, and b.m.o is upgraded, 9 seconds a night seems to be minimal, and my patch only outputs stuff to stderr if a problem was found. I got another mail yesterday, containing my changes + a state change on a dependancy from 10 days ago. It doesn't happen freqently, but I think we should try to track it down. After the first run, anyway. Almost 10% of bugs have invalid times on them? Ouch, and more ouch since this only covers bugs since newemailtech was checked in (Jan 2000 into cvs). I suppose most of these are from removing keywords. It still seems really high, though. justdave, any ideas?

We need to make sure rescanall doesn't touch anything within the last hour to avoid the situation where you make a change, then the rescanall processes the change, then you call processmail for the change.

Moving to 2.16. This is a pretty serious problem and we should recommend running rescanall nightly in the release notes.

Well, this doesn't actually fix the bug, it just makes it not matter. I blame dependancies' status changes, and will go do some poking, I think. And some people somehow have last_diffed = 00-00-00 00:00:00. I have no idea how that is possible. I should probably modify it to also print if any mail was actually sent, if we're running rescanall.

Actually, the last hour thing won't result in duplicate mails, because of the locks we have. However, it could, in theory, give a few false positives, so I'll change it. 15 minutes should be way more than enough, though.

I'm not sure what false positives here. I was concerned that if rescanall handled it, the UI would not get supplied the userlist and error messages that processmail gives it.

OK, rescanall will now only check for the last half hour. Patch coming.

I assume you mean won't check for the last half hour?

err, yeah. That. :)

While doing some pre-review work, I found that the following files contain SQL that updates the BUGS table, but does not contain the "delta_ts = delta_ts" (or other "detla_ts = foo") clause: CGI.pl [1 instance] move.pl [2 instances, together] process_bug.cgi [4 instances, one in global $::query variable] Also, the file "UPGRADING-pre-2.8" contains some SQL that pre-2.8 users must run which also contains many "UPDATE BUGS ..." SQL statements. I'm not sure if it's necessary to update this document as well, but I thought I'd mention it just in case. Does the SQL in the above cases need to be updated? I guess I'm not clear on when "delta_ts" should or should not be set to itself or something else.

Adding myself to CC list for r= purposes.

I thought I checked them all. I'll cehck when I get back home later this weekend. I have no idea about pre2.8. Do we still support upgrading from that version? Do the instructions still work?

ddk: Nope, those places are where we want the change to be recorded (because we're adding a comment/keyword/etc and so need the time moved so its picked up in the diff)

Comment on attachment 55497 [details] [diff] [review] patch, v2 Looks good! I ran this patch on my local installation and it picked up a large number of false positives in sanitycheck.cgi. Running "processmail rescanall" fixed them.

Regarding the changing of "UPGRADING-pre-2.8", I don't think this is necessary since (1) I'm not sure if a delta_ts field existed in the BUGS table at that time and (2) if the user manages to successfully upgrade from pre-2.8 to 2.15 (or newer), a quick run of sanitycheck.cgi will show the "unsent email" (some of which may be false positives), and will tell the user how to clear them ("processmail rescanall").

r=ddk for attachment 55497 [details] [diff] [review] (patch, v2). I didn't explictly state that in the comments I made when I reviewed the patch.

The code looks good, but I have a few questions and comments: 1. How will processmail rescanall not send mail the first time it encounters all those false positives? It looks to me like the code does send mail in that situation. 2. It's unclear to me that the problem with the edit*.cgi routines is that they fail to maintain the value of delta_ts. If I move one or more bugs from one component to another, those bugs' constituencies will receive mail. Shouldn't this also be the case when I change the name of a component? 3. What does adding zero days to the timestamp get us in "date_add(delta_ts, interval 0 day)"?

1) It won't send mail - in the case of a false positve we will try to, but the query on the activity log will be ampty, and so mail will not be sent. 2) No mail is sent when a component is renamed - it doesn't appear on the activity log. This is probably how these false positives are showing up, we think. 3) It formats teh date from a unix timestamp into a readable format - that value is only used for teh debugging output printed to STDERR

>2) No mail is sent when a component is renamed - it doesn't appear on the >activity log. This is probably how these false positives are showing up, we >think. Right, I understand. My question is whether these renamings *should* be recorded in the activity log and an email *should* be sent. After all, the name of the component is changing just as surely as if I had moved the bugs from one component to another. Don't people need to know the name about the name change? Note: that was not a leading question. I haven't made up my mind yet either way, but we should think about this. Perhaps the optimal solution is to give administrators the option of whether or not to spam everyone when they make these changes, but if we don't do that, then what's the next best solution? >3) It formats teh date from a unix timestamp into a readable format - that >value is only used for teh debugging output printed to STDERR In that case it would make more sense to use FROM_UNIXTIME for clarity and consistency. http://mysql.com/doc/D/a/Date_and_time_functions.html

2) Well, thats a different issue. really. 3) There was some reason why that didn't work, I think. I'll play with it later, and try to recall what it was. It works on landfill, but maybe it wasn't available on some earlier mysql version. I'll check.

Please note that one bug 43600 is FIXED, changing a component will no longer require touching each bug in that component and thus not update the timestamp.

Myk: bug #97733.

The reson I was doing the addition was because mysql outputs timestamp vars as YYYYMMDDHHMMSS. I've added the appropriate punctuation in, now I also cleaed up the errors a bit. An admin can run this nightly redirecting stdout to /dev/null, and only needing to look at stderr to see if there were problems. OTOH, I haven't seen this problem on bmo for ages.

Hmm. processmail doesn't seem to grab any locks. Is that a (separate) issue?

Comment on attachment 65969 [details] [diff] [review] v3 Ok, we can work out those "should we send change mail for these kinds of changes" issues later in bug 97733 or its relatives, and the time stuff looks good. One more problem... >+ if ($#list > 0) { >+ print STDERR "$#list bugs found with possibly unsent mail\n"; >+ print STDERR "Updating bugs, sending mail if required\n"; >+ } else { >+ print "All appropriate mail appears to have been sent\n" >+ } $#list returns the index of the last element in the array, not the number of elements in it, so if @list contains one element $#list will equal zero (but there will still be one bug will possibly unsent mail). Use scalar(@list) instead to get the number of elements.

Comment on attachment 67568 [details] [diff] [review] v4 >Index: processmail >+ if (scalar(@list) > 0) { >+ print STDERR "$#list bugs found with possibly unsent mail\n"; >+ print STDERR "Updating bugs, sending mail if required\n"; >+ } else { >+ print "All appropriate mail appears to have been sent\n" >+ } Same comment as Myk's previous one. You need to use scalar(@list) here instead of $#list (in the text you're printing to STDERR as well). Otherwise you'll get an error message reporting one less bug than was actually found. FWIW (and this is used in several other places in Bugzilla as well) the scalar() part is implied if the left side of the comparison is a scalar. For example you can do: if (0 < @list) { and that will work. Can't do that in a string though... have to use scalar() and concatenate. print STDERR scalar(@list) . " bugs found with possibly unsent mail\n";

Comment on attachment 72245 [details] [diff] [review] v5 r= justdave looks good to me now. it passes tests, and rescanall finds bugs with mismatched timestamps and sends mail.

Comment on attachment 72245 [details] [diff] [review] v5 +if (@badbugs > 0) { + Alert("Bugs that have changes but no mail sent for at least half an hour: " . + join (", ", @badbugs)); + print("Run <code>processmail rescanall</code> to fix this<p>\n"); +} Nit: |(scalar(@badbugs) > 0| may be used optionally here for clarification, too, but the context makes the call to scalar() implied. r=ddk

Oops. I checked this in at 2002/03/03 22:06:20 - forgot to mark it fixed.

I just upgraded from 2.10 to 2.16.3. I ran the sanity check and under Checking for unsent mail there are many thousands of bugs listed, nearly all in our database. If I run Run processmail rescanall to fix this... [bugs@box bugzilla]$perl processmail rescan will this send out emails for all of these bugs? I was thinking of changing the extension on the emails in the parameters so that I don't bombard everyone. Is this the correct way to run processmail and what do you recommend I do?

The "emails with unsent mail" is actually any bug where there is more than a half hour difference between the "last mail sent" timestamp and the "last modified" timestamp. There were bugs at various points in Bugzilla's history (many were just corrected in 2.16) where changes would get made to a bug and it wouldn't update one of the timestamps. In most cases, the timestamp difference is probably a phantom. In the real world when that first became available, I remember having it claim there were about 1500 bugs with unsent mail on one of my installs, and it only actually sent mail on about 20 of them. (There were only 20 where there were any activity records on that bug between the two timestamps). YMMV of course.

actually, I had that backwards -- the bugs were situations where the last-modified date on the bug WOULD get updated when there weren't actually modifications made to the bug...