Last Comment Bug 257351 - collectstats.pl dies if an invalid series is defined
: collectstats.pl dies if an invalid series is defined
Status: RESOLVED FIXED
: crash, dataloss
Product: Bugzilla
Classification: Server Software
Component: Reporting/Charting (show other bugs)
: 2.19
: All All
: -- major (vote)
: Bugzilla 2.20
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
: 392247 (view as bug list)
Depends on: 257344
Blocks: 257346 meta-pg
  Show dependency treegraph
 
Reported: 2004-08-29 10:45 PDT by Joel Peshkin
Modified: 2008-01-04 06:05 PST (History)
3 users (show)
mkanat: approval+
mkanat: blocking3.1.1-
mkanat: approval3.0+
mkanat: blocking3.0.1-
mkanat: approval2.22+
mkanat: blocking2.22.3-
mkanat: approval2.20+
mkanat: blocking2.20.5-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (2.46 KB, patch)
2007-08-14 15:35 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2 (2.58 KB, patch)
2007-08-14 15:52 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v3 (2.38 KB, patch)
2007-08-17 17:48 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Joel Peshkin 2004-08-29 10:45:49 PDT
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().
Comment 1 Joel Peshkin 2005-04-04 16:00:11 PDT
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.
Comment 2 Frédéric Buclin 2007-08-14 15:31:24 PDT
*** Bug 392247 has been marked as a duplicate of this bug. ***
Comment 3 Frédéric Buclin 2007-08-14 15:35:31 PDT
Created attachment 276704 [details] [diff] [review]
patch, v1

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.
Comment 4 Frédéric Buclin 2007-08-14 15:38:53 PDT
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.
Comment 5 Frédéric Buclin 2007-08-14 15:52:24 PDT
Created attachment 276710 [details] [diff] [review]
patch, v2

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.
Comment 6 Frédéric Buclin 2007-08-14 15:58:42 PDT
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 Max Kanat-Alexander 2007-08-14 17:15:49 PDT
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.
Comment 8 Frédéric Buclin 2007-08-15 03:02:12 PDT
(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 Reed Loden [:reed] (use needinfo?) 2007-08-15 03:04:30 PDT
(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. :)
Comment 10 Max Kanat-Alexander 2007-08-17 17:38:12 PDT
Comment on attachment 276710 [details] [diff] [review]
patch, v2

Just fix sql_group_by to use "if" instead of "if defined".
Comment 11 Frédéric Buclin 2007-08-17 17:48:55 PDT
Created attachment 277187 [details] [diff] [review]
patch, v3
Comment 12 Max Kanat-Alexander 2007-08-18 13:55:27 PDT
Comment on attachment 277187 [details] [diff] [review]
patch, v3

Looks right to me! r=mkanat by inspection.
Comment 13 Frédéric Buclin 2007-08-20 11:12:02 PDT
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
Comment 14 Frédéric Buclin 2008-01-04 06:05:49 PST
Has been relnoted when releasing Bugzilla 3.0.1, 3.1.1, 2.22.3 and 2.20.5.

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