276237 - Charting is completely broken - can't add datasets to list

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Gervase Markham [:gerv]]

New Charts are broken on b.m.o. - you can't add any datasets to your list. They
work on my local install and on landfill - both the tip and the 2.18 branch. So
it must be something about b.m.o. specifically.

I've tailed the webserver error log, but I can't see anything there. The only
thing I can think of is that it might be dropping the data sets you try and add
for security reasons - but the category_group_map table is completely empty.

I'm somewhat surprised no-one has complained about this. Does no-one use this
function at all?

Gerv

Comment 1

[:glob ✱]

i just created a dataset named "test", and i'm able to add that to the list.
perhaps the existing datasets were not upgraded correctly.

Comment 2

[Max Kanat-Alexander]

Does this mean that chart upgrading actually doesn't work?

If so, we really can't release 2.18 like that.

Comment 3

[Dave Miller [:justdave]]

I'm going to attack this later tonight...   I have a backup copy of the b.m.o
database from immediately prior to the upgrade.  I'm going to restore that to an
alternate database name and let checksetup.pl have a crack at it and poke around
to see exactly what it touches.

Comment 4

[Dave Miller [:justdave]]

OK, got the backup out, and ran checksetup.pl on it...

and we have a winner (I think)

Here's why charts are broken, I suspect:

Adding new field public to table series ...
DBD::mysql::db do failed: Unknown table 'user_series_map' in where clause at
./checksetup.pl line 3976.

Don't know how I ever missed that when we upgraded.

Comment 5

[Gervase Markham [:gerv]]

> Adding new field public to table series ...
> DBD::mysql::db do failed: Unknown table 'user_series_map' in where clause at
> ./checksetup.pl line 3976.

That's incredibly weird. The line of code concerned is wrapped in an
if (TableExists("user_series_map")) {
block! And the deletion of the table doesn't happen until a line further down.

Dave: can you puzzle out what's going on here?

Gerv

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> That's incredibly weird. The line of code concerned is wrapped in an
> if (TableExists("user_series_map")) {
> block!

  Actually, at least on my 2.18, that code is in the block *below* the if
(TableExists) block. That is, the TableExists starts on 3855, and ends on 3874.

The code mentioned is in a "if (!$series_exists) {" block.

Comment 7

[Max Kanat-Alexander]

My best buess at this point is that the two blocks are reversed, and TableExists
should actually go after the series creation.

Comment 8

[Gervase Markham [:gerv]]

I've just completely updated my trunk and 2.18 trees, and checked my
checksetup.pl files have no diffs.

On the trunk, we have:

3981: if (TableExists("user_series_map")) {
      ...

3994:    # Migrate public-ness across from user_series_map to new field
         $dbh->do("UPDATE series SET series.public = 1 " .
                  "WHERE series.series_id = user_series_map.series_id " .
                  "  AND user_series_map.user_id = 0");

3999:    $dbh->do("DROP TABLE user_series_map");
     }    

And on 2.18:

3848: if (TableExists("user_series_map")) {
      ...
3861:    # Migrate public-ness across from user_series_map to new field
         $dbh->do("UPDATE series SET series.public = 1 " .
                  "WHERE series.series_id = user_series_map.series_id " .
                  "  AND user_series_map.user_id = 0");

3866:    $dbh->do("DROP TABLE user_series_map");
     }    

In both cases, these are the the only instances of "user_series_map" in the
entire file.

If anyone else is seeing anything different, something weird is going on.

Gerv

Comment 9

[Nick Barnes]

Problem is, the SQL which sets series.public is broken:

    # Migrate public-ness across from user_series_map to new field
    $dbh->do("UPDATE series SET series.public = 1 " .
             "WHERE series.series_id = user_series_map.series_id " .
             "  AND user_series_map.user_id = 0");

Putting user_series_map in the WHERE clause like that just isn't going to work.
In a real SQL you could use a subselect for this:

 update series set public = 1 where series_id in (select series_id from
user_series_map where user_id = 0)

but we have to work in old MySQLs which don't have subselects, so we have to do
it in Perl.  Patch forthcoming.  BTW, I'm not sure whether this addresses the
original bug here.

Comment 10

[Nick Barnes]

(In reply to comment #9)
> Problem is, the SQL which sets series.public is broken:
> 
>     # Migrate public-ness across from user_series_map to new field
>     $dbh->do("UPDATE series SET series.public = 1 " .
>              "WHERE series.series_id = user_series_map.series_id " .
>              "  AND user_series_map.user_id = 0");
> 
> Putting user_series_map in the WHERE clause like that just isn't going to work.

The effect of this when upgrading a 2.17.7 database using a current 2.18 branch
checksetup.pl is that none of the series are public.

I now have a patch.

Comment 11

[Nick Barnes]

Attachment: Patch for 2.18 branch tip

This patch fixes checksetup so that any series with user_series_map.user_id = 0
gets series.public set to 1.  Without this patch, checksetup.pl complains and
series.public is set to 0 for all series.

Whether this fixes the original bug here is beyond me.

Comment 12

[Gervase Markham [:gerv]]

> Putting user_series_map in the WHERE clause like that just isn't going to work.

I'm completely flummoxed by that statement. I've looked at that SQL again and
again, but all I keep seeing is a really simple bit of SQL which works with
every database out there, of the form UPDATE something SET something WHERE
something else IS TRUE. What makes it require subselects? Why is this not
utterly standard and normal?

Gerv

Comment 13

[Nick Barnes]

(In reply to comment #12)
> > Putting user_series_map in the WHERE clause like that just isn't going to work.
> 
> I'm completely flummoxed by that statement. I've looked at that SQL again and
> again, but all I keep seeing is a really simple bit of SQL which works with
> every database out there, of the form UPDATE something SET something WHERE
> something else IS TRUE. What makes it require subselects? Why is this not
> utterly standard and normal?

In the WHERE clause you say
    WHERE series.series_id = user_series_map.series_id AND
user_series_map.user_id = 0

but user_series_map isn't a table to which you are otherwise referring in the
UPDATE statement.  In other words, it's an unbound table variable here.
I'm sure there are many ways of doing this in other SQLs, but I would do it with
a subselect (i.e. "where series_id in (SELECT series_id from user_series_map
where user_id = 0)").
AFAIK you Just Can't Do It in MySQL (3.x), so you have to collect the series_id
values which you're interested in and then squirt them back into SQL.

By the way, my patch is far from perfect.  Better to prepare() the UPDATE
statement.  But it works.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 170822 [details] [diff] [review]
Patch for 2.18 branch tip

Yes, this is correct, on inspection. I would have used a selectcol_arrayref and
then done a foreach, but this is basically the same thing. :-)

justdave -- Does this fix the problem on the copy of the b.m.o database?

Comment 15

[Max Kanat-Alexander]

Comment on attachment 170822 [details] [diff] [review]
Patch for 2.18 branch tip

OK, nevermind. I just found out how to reproduce it on landfill. I just have to
upgrade from a 2.17.6 install, and the error shows up every time.

The patch definitely fixes the error, and I'd imagine that having no public
series could be a problem. :-)

Comment 16

[Dave Miller [:justdave]]

This needs to be release-noted.  Correcting the problem on a site that's already
been bitten by it requires restoring a backup of the user_series_map table from
prior to the upgrade.  Woe be the site that didn't save their backup.

Comment 17

[Gervase Markham [:gerv]]

A secondary option for sites who have not yet created any new charts, or which
are willing to lose the data collected thusfar, would be to drop the 'series',
'series_data', 'series_categories' and 'category_group_map' tables entirely, and
let the data be re-migrated from Old Charts.

A third possible option, if they aren't using groups, is 
UPDATE series SET series.public = 1

Gerv

Comment 18

[Max Kanat-Alexander]

How to Fix The New Charts Problem
---------------------------------
If you updated from a version between 2.17.4 - 2.18rc2 to either 2.18rc3 or
2.19.1, your "New Charts" may not work. There are three different possible ways
to fix the problem, depending on your situation:


If You Aren't Using Chart Security
----------------------------------
If you aren't controlling who can see what charts by the groups system, just run
the following SQL on your Bugzilla database:

UPDATE series SET series.public = 1;


If You Haven't Yet Used New Charts
----------------------------------
You can run the following command on your DB to re-migrate data from the Old Charts:

DROP TABLE series, series_data, 'series_categories', 'category_group_map';

Then re-run checksetup.pl.


If You Have Used New Charts, And Are Using Security On Them
-----------------------------------------------------------
You will have to restore the user_series_map table from a backup from before
your upgrade and add it into your current DB. Then run checksetup.pl and it will
properly set up security for your New Charts so that they can be used.

[These instructions here so that I can reference them from the release notes.]

Comment 19

[Max Kanat-Alexander]

For the second solution, the command should actually be:

DROP TABLE series, series_data, series_categories, category_group_map;

(Just remove the single-quotes.)

Comment 20

[Max Kanat-Alexander]

(Relnote written, removing things from this bug that were about the relnote.)

Comment 21

[Dave Miller [:justdave]]

Comment on attachment 170822 [details] [diff] [review]
Patch for 2.18 branch tip

applied patch to b.m.o, restored backup of the user_series_map table, ran
checksetup.pl, charts work now. :)

Comment 22

[timeless]

Comment on attachment 170822 [details] [diff] [review]
Patch for 2.18 branch tip

mozilla/webtools/bugzilla/checksetup.pl 	1.326
mozilla/webtools/bugzilla/checksetup.pl 	1.289.2.24

Comment 23

[Ishaan]

(spam deleted)