257351 - collectstats.pl dies if an invalid series is defined

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Joel Peshkin]

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]

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 3

[Frédéric Buclin]

Attachment: 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]

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]

Attachment: 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]

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]

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]

(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]]

(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]

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]

Attachment: patch, v3

Comment 12

[Max Kanat-Alexander]

Comment on attachment 277187 [details] [diff] [review]
patch, v3

Looks right to me! r=mkanat by inspection.

Comment 13

[Frédéric Buclin]

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]

Has been relnoted when releasing Bugzilla 3.0.1, 3.1.1, 2.22.3 and 2.20.5.