Bug 114353 - Bugzilla sends double (two) changes in one email
Bugzilla sends double (two) changes in one email
Status: CLOSED CURRENTRELEASE
Product: Bugzilla
Classification: Community
Component: Bugzilla General (Show other bugs)
2.17
All Linux
medium Severity medium (vote)
: ---
: ---
Assigned To: David Lawrence
David Lawrence
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2004-01-26 21:00 EST by Maxwell Kanat-Alexander
Modified: 2007-04-18 13:02 EDT (History)
0 users

See Also:
Fixed In Version: 2.17.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2004-02-10 13:36:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Changes lastdiffed to microseconds everywhere (6.48 KB, patch)
2004-01-27 19:56 EST, Maxwell Kanat-Alexander
no flags Details | Diff
Fixes slight problem with previous patch (5.74 KB, patch)
2004-02-03 14:53 EST, Maxwell Kanat-Alexander
no flags Details | Diff

  None (edit)
Description Maxwell Kanat-Alexander 2004-01-26 21:00:39 EST
I didn't see a bug for this, so I'll go ahead and file it.

The basic problem is that lastdiffed is being filed without
microseconds. This is actually a big problem all over the Red Hat
bugzilla. So, everywhere that it's compared to a normal timestamp,
things go wrong.

I'm working on it, now.

Here's what dkl sent me, originally:

The code is in processmail and has to do with the way that does date
math differently than mysql. If you look for

    if ($::db_driver eq 'Pg') {
        # PostgreSQL does not seem to like empty dates or dates that
are all 0's
        $start = '1970-01-01 12:01:01' if !$start; # Set to some
really early date
        $datepart = " AND bug_when > TO_TIMESTAMP('$start','YYYY-MM-DD
HH24:MI:SS') " .
                    " AND bug_when < TO_TIMESTAMP('$end','YYYY-MM-DD
HH24:MI:SS') + interval '1 sec' ";
    } else {
        $datepart = "  AND bug_when > '$start' AND bug_when <= '$end' ";
    }
                                                                     
                                                                     
                 
    SendSQL("SELECT profiles.login_name, fielddefs.description, " .
            "       bug_when, removed, added, attach_id,
fielddefs.name " .
            "FROM bugs_activity, fielddefs, profiles " .
            "WHERE bug_id = $id " .
            "  AND fielddefs.fieldid = bugs_activity.fieldid " .
            "  AND profiles.userid = who " .
            $datepart .
            "ORDER BY bug_when"
            );

-- snip --

SendSQL("SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, " .
            "       removed, added " .
            "FROM bugs_activity, bugs, dependencies, fielddefs ".
            "WHERE bugs_activity.bug_id = dependencies.dependson " .
            "  AND bugs.bug_id = bugs_activity.bug_id ".
            "  AND dependencies.blocked = $id " .
            "  AND fielddefs.fieldid = bugs_activity.fieldid" .
            "  AND (fielddefs.name = 'bug_status' " .
            "    OR fielddefs.name = 'resolution') " .
            $datepart .
            "ORDER BY bug_when, bug_id");

-- snip --

my ($newcomments, $anyprivate) = GetLongDescriptionAsText($id, $start,
$end);

you will see how much different they are. I have had to tweak that
area on occasion and still every once in a while see double changes
but hasn't been as bad. I have been bogged down with other stuff
lately and plan on getting back to this problem soon.

If you see of a better fix also let me know.
Comment 1 Maxwell Kanat-Alexander 2004-01-27 19:18:04 EST
Okay, I have a patch that seems to fix this. I'll work on creating it
and then post it. -Max
Comment 2 Maxwell Kanat-Alexander 2004-01-27 19:56:24 EST
Created attachment 97288 [details]
Changes lastdiffed to microseconds everywhere

Okay, this patch changes Bugzilla to use and expect lastdiffed as microseconds
everywhere. From my current testing, this completely eliminates the
double-comments problem, without any adverse effects. 

(The only adverse effect may happen when the database contains both comments w/
microseconds and those without. That is, after you apply this patch, the
*first* new email sent on any currently existing bug has a 50% chance of having
double comments. However, after that the double-comments will not happen.)

I will be applying this patch to my Bugzilla installation, and I'll let you
know how it goes.

-Max
Comment 3 Maxwell Kanat-Alexander 2004-02-02 12:44:45 EST
We've been running this code for a few days now, and it works. -Max
Comment 4 David Lawrence 2004-02-02 12:50:36 EST
Thanks for looking into this. I will get the patch rolled into our
environment here and let you know how it goes.
Comment 5 Maxwell Kanat-Alexander 2004-02-02 13:55:49 EST
Note that this breaks Sanity Check for emails -- I'll look into it. -Max
Comment 6 Maxwell Kanat-Alexander 2004-02-02 14:10:02 EST
Okay, I would recommend that before you apply the patch, you add 1
second to lastdiffed for every bug in the database. This should
prevent the initial duplicate emails, too. -Max
Comment 7 David Lawrence 2004-02-03 14:53:34 EST
I have added the patch to a devel instance of bugzilla here. Testing
to make sure that email is still being sent. You can disregard this email.

Comment 8 Maxwell Kanat-Alexander 2004-02-03 14:53:42 EST
Created attachment 97446 [details]
Fixes slight problem with previous patch

When I applied this patch, it exposed another bug, that we were setting
delta_ts in processmail when we shouldn't have been. That's the only change
between that patch and this, I believe.

Remember to add 1 second to lastdiffed for every bug in the bugs table before
you apply this patch.

-M
Comment 9 David Lawrence 2004-02-03 15:01:12 EST
Another test comment using version 2 of patch.
Comment 10 Maxwell Kanat-Alexander 2004-02-03 15:04:06 EST
Oh, the new patch also makes sanity check work correctly. :-) -M
Comment 11 Maxwell Kanat-Alexander 2004-02-03 15:14:52 EST
I should qualify that statement, slightly, and explain what the
problem was in more detail:

In processmail, there was special code for MySQL to *not* set delta_ts
when lastdiffed was set. Not knowing MySQL inside and out, we
mis-interpreted this as "we have to set delta_ts here", and set it to
"now()". This caused delta_ts to be set to 1/2 second later than
lastdiffed, which made emails send correctly, but sanity check
complained, since lastdiffed *was* less than delta_ts.

The new version of the patch doesn't set delta_ts when lastdiffed is
set, but leaves it as it was. So, lastdiffed and delta_ts are correct
to the microsecond, now. If email is sent, lastdiffed will always be
greater than delta_ts.

This means that sanity check doesn't need to truncate delta_ts to the
second, but instead should use the microseconds. In fact, the original
Red Hat version did this, I merely changed it (as a workaround) in the
first patch. So the second patch sets that back to normal.

If you used the first patch, it would appear that some bugs were not
sending email (in sanity check) when in fact they had.

Adding 1 second to lastdiffed should solve the legacy problem, and the
new patch should prevent it from happening in the future.

-M
Comment 12 David Lawrence 2004-02-03 15:19:13 EST
I seem to recall the reason for setting delta_ts explicitly was
because Mysql had a special column time called 'timestamp' that was
automatically updated when ever an UPDATE was done to a particular
row. PostgreSQL (last time I looked through the docs) did not have
something comparable so I did the update manually. Is this what you
are referring to?
Comment 13 David Lawrence 2004-02-03 15:32:22 EST
Okay, added to live site. Crossing fingers. Will close this out after
a couple days.
Comment 14 Maxwell Kanat-Alexander 2004-02-03 15:35:36 EST
Yeah, MySQL does that update timestamp thing. In processmail, though,
delta_ts is explicitly not supposed to be set. In fact, it wasn't
entirely me that noticed this -- I pasted some code to justdave and he
realized what was going wrong.

Here's the code in MySQL that sets lastdiffed in processmail:

UPDATE bugs SET lastdiffed = '$end', delta_ts = delta_ts

That actually prevents the MySQL trigger from firing, so delta_ts
maintains its old value. (We were doing "delta_ts=now()", which was
the opposite of what we were supposed to be doing. I just took out the
update of delta_ts entirely.)

Thankfully for us, the Mozilla Bugzilla was already setting delta_ts
manually, in process_bug.cgi:

UPDATE bugs SET delta_ts=NOW() WHERE bug_id=$i

justdave said that he did that because there are things like CC that
don't touch the bugs table, but delta_ts still needs to be updated.
It's just convenient for us that he did it in the fashion he did,
since it means that we get a free update for delta_ts where it's
supposed to happen.

If things besides process_bug touch the table, and we were depending
on processmail to set delta_ts, then that might cause some trouble.
Except that I think that the Mozilla Bugzilla actually does the above
*everywhere* that delta_ts is supposed to get updated, just out of
coincidence.

-M
Comment 15 David Lawrence 2004-02-03 15:42:18 EST
Ah that explains it. Was wondering about the delta_ts=delta_ts part
and now I know.

Comment 16 David Lawrence 2004-02-10 13:36:29 EST
Closing this out since everything has been working fine for several
days now. Thanks again for the patch.

Note You need to log in before you can comment on or make changes to this bug.