106377 - processmail rescanall should use lastdiffed

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

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.

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 2

[Myk Melez [:myk] [@mykmelez]]

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

Comment 3

[Bradley Baetz (:bbaetz)]

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?

Comment 4

[Matthew Tuck [:CodeMachine]]

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.

Comment 5

[Matthew Tuck [:CodeMachine]]

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

Comment 6

[Bradley Baetz (:bbaetz)]

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.

Comment 7

[Bradley Baetz (:bbaetz)]

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.

Comment 8

[Matthew Tuck [:CodeMachine]]

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.

Comment 9

[Bradley Baetz (:bbaetz)]

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

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: patch, v2

Comment 11

[Matthew Tuck [:CodeMachine]]

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

Comment 12

[Bradley Baetz (:bbaetz)]

err, yeah. That. :)

Comment 13

[David D. Kilzer (ddk)]

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.

Comment 14

[David D. Kilzer (ddk)]

Adding myself to CC list for r= purposes.

Comment 15

[Bradley Baetz (:bbaetz)]

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?

Comment 16

[Bradley Baetz (:bbaetz)]

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 17

[David D. Kilzer (ddk)]

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.

Comment 18

[David D. Kilzer (ddk)]

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").

Comment 19

[David D. Kilzer (ddk)]

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.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

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)"?

Comment 21

[Bradley Baetz (:bbaetz)]

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

Comment 22

[Myk Melez [:myk] [@mykmelez]]

>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

Comment 23

[Bradley Baetz (:bbaetz)]

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.

Comment 24

[Jacob Steenhagen]

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.

Comment 25

[Matthew Tuck [:CodeMachine]]

Myk: bug #97733.

Comment 26

[Bradley Baetz (:bbaetz)]

Attachment: v3

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.

Comment 27

[Bradley Baetz (:bbaetz)]

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

Comment 28

[Myk Melez [:myk] [@mykmelez]]

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 29

[Bradley Baetz (:bbaetz)]

Attachment: v4

Comment 30

[Dave Miller [:justdave]]

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 31

[Bradley Baetz (:bbaetz)]

Attachment: v5

Comment 32

[Dave Miller [:justdave]]

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 33

[David D. Kilzer (ddk)]

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

Comment 34

[Bradley Baetz (:bbaetz)]

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

Comment 35

[dan johnson]

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?

Comment 36

[Dave Miller [:justdave]]

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.

Comment 37

[Dave Miller [:justdave]]

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...