Closed
Bug 257351
Opened 20 years ago
Closed 17 years ago
collectstats.pl dies if an invalid series is defined
Categories
(Bugzilla :: Reporting/Charting, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: bugreport, Assigned: LpSolit)
References
Details
(Keywords: crash, dataloss)
Attachments
(1 file, 2 obsolete files)
2.38 KB,
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
Collectstats still cannot recover from anything that will cause new Bugzilla::Search or $search->getSQL() to fail. There are several reasons for this... 1) There is no eval { } surrounding that 2) even if there were, bug 257344 would keep us from catching it It might be possible to add a parameter to new Bugzilla::Search that would indicate that we want a die() instead of exit().
Reporter | ||
Comment 1•19 years ago
|
||
By setting Bugzilla->batch = 1 prior to calling Bugzilla::Search, it will become possible to trap any errors it throws. After that, you can eval { } the call to Bugzilla::Search (that only traps errors, it does not compile at run-time as long as you eval a block not a string) and the call to execute the SQL. It is best tested by hacking search to force an error and then mangling the SQL to force an error.
Updated•18 years ago
|
QA Contact: mattyt-bugzilla → default-qa
Assignee | ||
Comment 3•17 years ago
|
||
This patch fixes 2 problems: - First, the original bug found by joel. If a query has invalid data, Search->new will throw an error which isn't caught by eval{} as it's called outside it. - Second, Search.pm incorrectly assumes that @groupby is never empty, which is wrong at least when called from collectstats.pl. This generates an invalid SQL query and PostgreSQL dies, meaning that most (all?) series fail on Pg.
Assignee | ||
Comment 4•17 years ago
|
||
Comment on attachment 276704 [details] [diff] [review] patch, v1 Hum, wait, no. My fix in Search.pm is incorrect. If @groupby is empty, nothing should be appended. Weird.
Attachment #276704 -
Flags: review?(justdave)
Assignee | ||
Comment 5•17 years ago
|
||
Fun, join(',', @a) is always defined, even if @a is empty. This was confusing sql_group_by() on PostgreSQL as it was appending a comma which generated an invalid SQL. MySQL is not affected as it ignores optional fields.
Attachment #276704 -
Attachment is obsolete: true
Attachment #276710 -
Flags: review?(justdave)
Assignee | ||
Comment 6•17 years ago
|
||
Series are silently skipped on PostgreSQL and cannot be created -> dataloss. Moreover, a renamed user account or product or component, etc... crashes collectstats.pl, preventing it from collecting data even for valid series -> dataloss + crash. So it worths taking it on all branches, including 2.20 and 2.22 (as we still accept dataloss + crash fixes there).
Comment 7•17 years ago
|
||
This is bad, but we're too far into the release process at this point for this to be a *blocker* on all these branches. Also, we discovered it internally, we didn't even have some user report it to us, so we don't even have evidence that anybody is experiencing it. There aren't even any users who CC'ed themselves on this bug. I've never seen a question about it on the support list. It would certainly be good to fix, but it's not a blocker this far into the release process.
Flags: blocking3.1.1?
Flags: blocking3.1.1-
Flags: blocking3.0.1?
Flags: blocking3.0.1-
Flags: blocking2.22.3?
Flags: blocking2.22.3-
Flags: blocking2.20.5?
Flags: blocking2.20.5-
Assignee | ||
Updated•17 years ago
|
Attachment #276710 -
Flags: review?(gerv)
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #7) > Also, we discovered it internally, we didn't even have some user report it to > us, so we don't even have evidence that anybody is experiencing it. There > aren't even any users who CC'ed themselves on this bug. I've never seen a > question about it on the support list. About this bug, either the Pg issue or collectstats.pl dying prematurely, justdave reported this problem yesterday on IRC, so it's not an "internal" thing only: <justdave> hmm, so collectstats.pl is dying during the series data collection on weekly queries on bmo <justdave> the error showing up in the cron mail reports an invalid email address ***justdave started looking for this because someone was complaining that their weekly series query wasn't running [... LpSolit and justdave then found the culprit query ...] <justdave> so failure to change login_names in series when someone changes their email address breaks collectstats.pl <LpSolit> it shouldn't as Search.pm is called within an eval() <justdave> as mentioned though, I had people complaining that their queries weren't running <LpSolit> oh, collectstats.pl is wrong <LpSolit> new Bugzilla::Search() is called outside eval() <LpSolit> but new() is the one doing the checks about passed data And I can confirm the problem above with my own series on b.m.o. My series have missing data every Sunday starting from July 15. If I were unlucky enough and was running my series every Sunday only, I would have no data for a month now, and there is *no way* to regenerate them in new charts. So even b.m.o is affected. Talking about b.m.o, reed, you should take this fix when you upgrade tonight.
Comment 9•17 years ago
|
||
(In reply to comment #8) > Talking about b.m.o, reed, you should take this fix when you upgrade tonight. Well, get it reviewed and committed on the branch by tonight, and it'll make it. :)
Assignee | ||
Updated•17 years ago
|
Attachment #276710 -
Flags: review?(justdave) → review?(mkanat)
Comment 10•17 years ago
|
||
Comment on attachment 276710 [details] [diff] [review] patch, v2 Just fix sql_group_by to use "if" instead of "if defined".
Attachment #276710 -
Flags: review?(mkanat)
Attachment #276710 -
Flags: review?(gerv)
Attachment #276710 -
Flags: review-
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #276710 -
Attachment is obsolete: true
Attachment #277187 -
Flags: review?(mkanat)
Comment 12•17 years ago
|
||
Comment on attachment 277187 [details] [diff] [review] patch, v3 Looks right to me! r=mkanat by inspection.
Attachment #277187 -
Flags: review?(mkanat) → review+
Updated•17 years ago
|
Flags: approval3.0+
Flags: approval2.22+
Flags: approval2.20+
Flags: approval+
Assignee | ||
Comment 13•17 years ago
|
||
tip: Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.62; previous revision: 1.61 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.102; previous revision: 1.101 done 3.0: Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.58.2.3; previous revision: 1.58.2.2 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.93.2.3; previous revision: 1.93.2.2 done 2.22.2: Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.46.2.1; previous revision: 1.46 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.71.2.5; previous revision: 1.71.2.4 done 2.20.4: Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.43.4.4; previous revision: 1.43.4.3 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.56.2.8; previous revision: 1.56.2.7 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Summary: collectstats dies if an invalid series is defined → collectstats.pl dies if an invalid series is defined
Assignee | ||
Comment 14•16 years ago
|
||
Has been relnoted when releasing Bugzilla 3.0.1, 3.1.1, 2.22.3 and 2.20.5.
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•