Bug 114353
| Summary: | Bugzilla sends double (two) changes in one email | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Community] Bugzilla | Reporter: | Maxwell Kanat-Alexander <mkanat> | ||||||
| Component: | Bugzilla General | Assignee: | David Lawrence <dkl> | ||||||
| Status: | CLOSED CURRENTRELEASE | QA Contact: | David Lawrence <dkl> | ||||||
| Severity: | medium | Docs Contact: | |||||||
| Priority: | medium | ||||||||
| Version: | 2.17 | ||||||||
| Target Milestone: | --- | ||||||||
| Target Release: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | Linux | ||||||||
| Whiteboard: | |||||||||
| Fixed In Version: | 2.17.1 | Doc Type: | Bug Fix | ||||||
| Doc Text: | Story Points: | --- | |||||||
| Clone Of: | Environment: | ||||||||
| Last Closed: | 2004-02-10 18:36:29 UTC | Type: | --- | ||||||
| Regression: | --- | Mount Type: | --- | ||||||
| Documentation: | --- | CRM: | |||||||
| Verified Versions: | Category: | --- | |||||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||||
| Embargoed: | |||||||||
| Attachments: |
|
||||||||
Okay, I have a patch that seems to fix this. I'll work on creating it and then post it. -Max 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
We've been running this code for a few days now, and it works. -Max Thanks for looking into this. I will get the patch rolled into our environment here and let you know how it goes. Note that this breaks Sanity Check for emails -- I'll look into it. -Max 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 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. 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
Another test comment using version 2 of patch. Oh, the new patch also makes sanity check work correctly. :-) -M 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 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? Okay, added to live site. Crossing fingers. Will close this out after a couple days. 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 Ah that explains it. Was wondering about the delta_ts=delta_ts part and now I know. Closing this out since everything has been working fine for several days now. Thanks again for the patch. |
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.