253696 - collectstats.pl crashes because of ActivePerl / DBI problem

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Aku Kauste]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9

Collectstats crashes on Windows + ActivePerl combination. I've tried both
Bugzilla 2.18rc1 & 2.18rc2.

This error is propably more an ActivePerl or DBI.pm problem, but there is an
easy workaround. I tried to grep and collectstats.pl is the only Bugzilla-file
that is using selectall_hashref method. 
To fix this bug replace lines 460-464 from collectstats.pl with this code:

my $series_sth = $dbh->prepare("SELECT series_id, query, creator " .
                 "FROM series " .
                 "WHERE frequency != 0 AND " . 
                 "($days_since_epoch + series_id) % frequency = 0");
$series_sth->execute();
my $serieses;
while ( my $row = $series_sth->fetchrow_hashref() ) {
	$$serieses{ $$row{"series_id"} } = $row;
}



Reproducible: Always
Steps to Reproduce:
1. Try to run collectstats.pl
AFAIK it only happens with Windows ActivePerl. I've tried with versions 809 and 810.
Actual Results:  
Free to wrong pool 15d27f8 not ff at C:/usr/site/lib/DBI.pm line 1867.

Comment 1

[Jouni Heikniemi]

Byron, didn't you help somebody with this same issue recently?

Aku: Can you please post your DBI version number (preferably from checksetup.pl
output)?

Comment 2

[Aku Kauste]

My DBI version is 1.43

Comment 3

[:glob ✱]

i did help someone out, but i'd always figured it was a problem with my specific
box.  i didn't hear back from the guy i was helping, and no news is good news.

Comment 4

[:glob ✱]

Attachment: fix activestate perl dbi crash

the change aku suggested looks good to me.

thanks aku :)

Comment 5

[Jouni Heikniemi]

The change by itself looks fine with be, but I'm slightly confused as to what's
the problem and what's the generic solution here. Does selectall_hashref
misbehave on Windows or a certain DBI version? If so, we should globally search
for problematic callsites or at least identify the scenario and post to
developers@, advicing people to avoid this. 

I can't try this on Win32 right now (just quit from my previous job yesterday,
so my test platform range is minimized atm), but can you guys try to isolate the
problem?

Comment 6

[Dave Miller [:justdave]]

yeah, we should find out what's killing it.  We can do an OS-specific version
check for a newer version of DBI when on Windows if we need to. 
selectall_hashref is a well-known DBI function, and it'll be hard to expect
people not to use it when it makes sense to.

Comment 7

[:glob ✱]

i have this working on one system, and crashing on another:

works -

XP Pro
ActivePerl 5.8.2 Build 808
DBI 1.43
DBD-mysql 2.9002
mysql v4.0.20a-debug

fails -

XP Pro
ActivePerl 5.8.4 Build 810
DBI 1.43
DBD-mysql 2.9002
mysql v4.0.20a-nt


aku: what build of perl are you running?

Comment 8

[:glob ✱]

downgrading perl from 5.8.4 build 810 to 5.8.2 build 808 fixes the crash.

Comment 9

[Aku Kauste]

Currently I have:
ActivePerl 5.8.4 Build 810

but first I had:
ActivePerl 5.8.3 Build 809

So the problem must be new to 809. 

Comment 10

[:glob ✱]

when connecting to the databse, if i don't set 
  FetchHashKeyName => 'NAME_lc'
or set it to 'NAME', it doesn't crash on 5.8.4

Comment 11

[Jouni Heikniemi]

Comment on attachment 154854 [details] [diff] [review]
fix activestate perl dbi crash

Marking r-; let's find the correct solution here and then patch that. So far,
your detective work is looking very good indeed.

Comment 12

[Dave Miller [:justdave]]

Possibly related:

http://bugs.activestate.com/show_bug.cgi?id=30642

Comment 13

[:glob ✱]

i've asked the DBI list about this, with a single response: "try google".  not
very useful.

so i've lodged it as a perlbug
http://guest:[e-mail]/rt3/Ticket/Display.html?id=30933


how about we drop FetchHashKeyName => 'NAME_lc' from the dbi object? the only
query it'll impact is the one that's causing this crash.

Comment 14

[:glob ✱]

Attachment: set FetchHashKeyName => 'NAME'

zero useful responses from dbi, perl and activestate support :(

this patch sets FetchHashKeyName to 'NAME', which means the keys in the hash
generated by fetchrow_hashref are in the same case as the columns in the
database.

ref: http://search.cpan.org/~timb/DBI/DBI.pm#FetchHashKeyName

this will affect the following functions:
  selectrow_hashref (unused in 2.18)
  selectall_hashref (collectstats only)
  fetchrow_hashref  (unused in 2.18)
  fetchall_arrayref (Bugzilla/Chart.pm, but not affected by change)

i've done some testing, and collectstats inserts the same rows into series_data
with FetchHashKeyName set to NAME and NAME_lc.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 158194 [details] [diff] [review]
set FetchHashKeyName => 'NAME'

Jouni, do you have time to review this 2.18 blocker?

Comment 16

[Jouni Heikniemi]

Comment on attachment 158194 [details] [diff] [review]
set FetchHashKeyName => 'NAME'

>-                             FetchHashKeyName => 'NAME_lc',
>                              TaintIn => 1,
>+                             FetchHashKeyName => 'NAME',  
>+                             # Note: NAME_lc causes crash on ActiveState Perl
>+                             # 5.8.4 (see Bug 253696)

The FetchHashKeyName option has the following documentation:

"This attribute is used to specify whether the fetchrow_hashref() method should
perform case conversion on the field names used for the hash keys. For
historical reasons it defaults to 'NAME' but it is recommended to set it to
'NAME_lc' (convert to lower case) or 'NAME_uc' (convert to upper case)
according to your preference. It can only be set for driver and database
handles. For statement handles the value is frozen when prepare() is called."

In the patch above, we're explicitly setting the default. I think that's quite
ok, as we have a special reason for doing that. The correct review for this
patch is to go through all instances where _hashref DBI functions are used and
check that none of them depend on SELECT Foo being accessed as
$result->{'foo'}. The only place we use _hashref stuff is in collectstats.pl,
and there we have the correct casing. Thus, I have good faith in the patch.

r=jouni

Comment 17

[Dave Miller [:justdave]]

(In reply to comment #16)
> The correct review for this
> patch is to go through all instances where _hashref DBI functions are used and
> check that none of them depend on SELECT Foo being accessed as
> $result->{'foo'}.

I also don't believe we have any columns anywhere in our schema with mixed-case
names...

Comment 18

[Bradley Baetz (:bbaetz)]

I don't think that this is a good idea; some DBD modules, or databases, may
case-noramlize names to a certain case if we don't explictly set it. Thats why
that warning is in there....

Comment 19

[Dave Miller [:justdave]]

ok, cancelling approvals on bbaetz' input.  You have any suggestions on a better
way to handle it?

Comment 20

[Jouni Heikniemi]

(In reply to comment #18)
> I don't think that this is a good idea; some DBD modules, or databases, may
> case-noramlize names to a certain case if we don't explictly set it. Thats why
> that warning is in there....

Do you have an example of such an implementation? I'd understand that under some
conditions the returned column names might be normalized to the schema-set
column case, but every database I've seen returns columns in the case used in
the SELECT statement. If the SELECT statement uses the same casing as the schema
(as it happens to be in Bugzilla), any case normalizations are IMO extremely
unlikely. 

Comment 21

[Bradley Baetz (:bbaetz)]

Oracle does, IIRC, at least within sqlplus. (I think; its been a while)

I guess we can leave it as 'NAME' with an appropriate comment, and wait til
someone ports bugzill toa DB which cares....

Comment 22

[Dave Miller [:justdave]]

(In reply to comment #21)
> I guess we can leave it as 'NAME' with an appropriate comment, and wait til
> someone ports bugzill toa DB which cares....

Works for me.  Make sure to add such a comment when you check it in on the
trunk. It can go in on the branch without it, since we won't have database
independence on the branch anyway.

Comment 23

[Jouni Heikniemi]

(In reply to comment #21)
> I guess we can leave it as 'NAME' with an appropriate comment, and wait til
> someone ports bugzill toa DB which cares....

Agreed. By the time that happens, it's also pretty likely somebody has fixed the
name_lc issue with AS Perl anyway. With the small number of Windows
installations at large and the fact that Perl isn't often used to run anything
else on those servers, I think we can bump version requirements immediately when
a working version of DBI appears in PPM.

I'll try Oracle/SQLPlus at work next week, interesting if it actually massages
names like that...

Comment 24

[:glob ✱]

can someone please check this in for me?

Comment 25

[Dave Miller [:justdave]]

Trunk:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.13; previous revision: 1.12
done

2.18 branch:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.12.2.1; previous revision: 1.12
done