collectstats.pl dies if an invalid series is defined

RESOLVED FIXED in Bugzilla 2.20

Status

()

defect
--
major
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: bugreport, Assigned: LpSolit)

Tracking

({crash, dataloss})

Dependency tree / graph
Bug Flags:
approval +
blocking3.1.1 -
approval3.0 +
blocking3.0.1 -
approval2.22 +
blocking2.22.3 -
approval2.20 +
blocking2.20.5 -

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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

14 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

13 years ago
QA Contact: mattyt-bugzilla → default-qa
(Assignee)

Updated

12 years ago
Duplicate of this bug: 392247
(Assignee)

Comment 3

12 years ago
Posted patch patch, v1 (obsolete) — Splinter Review
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: gerv → LpSolit
Status: NEW → ASSIGNED
Attachment #276704 - Flags: review?(justdave)
(Assignee)

Comment 4

12 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

12 years ago
Posted patch patch, v2 (obsolete) — Splinter Review
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

12 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).
Severity: normal → major
Flags: blocking3.1.1?
Flags: blocking3.0.1?
Flags: blocking2.22.3?
Flags: blocking2.20.5?
Keywords: crash, dataloss, relnote
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Updated

12 years ago
Blocks: meta-pg
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

12 years ago
Attachment #276710 - Flags: review?(gerv)
(Assignee)

Comment 8

12 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.
(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

12 years ago
Attachment #276710 - Flags: review?(justdave) → review?(mkanat)
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

12 years ago
Posted patch patch, v3Splinter Review
Attachment #276710 - Attachment is obsolete: true
Attachment #277187 - Flags: review?(mkanat)
Comment on attachment 277187 [details] [diff] [review]
patch, v3

Looks right to me! r=mkanat by inspection.
Attachment #277187 - Flags: review?(mkanat) → review+

Updated

12 years ago
Flags: approval3.0+
Flags: approval2.22+
Flags: approval2.20+
Flags: approval+
(Assignee)

Comment 13

12 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
Last Resolved: 12 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

11 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.