Bugzilla module containing small routines that can be used to hold database specific functionality. This keeps code changes to a minimum throughout the rest of the Bugzilla code. For more details about this module please refer to the comments on bug 98304. This report is being created to allow easier review in hopes of inclusion in 2.18. There will be a separate report for the remainder of the code changes for better MySQL/PostgreSQL compatibility.

Comment on attachment 144202 [details] DBcompat.pm module containing database compatibilty functions My first thought on this is that the commented descriptions above each sub are much easier to follow than the stuff in the pod docs at the bottom... it's also perfectly legal to have the pod docs be inline interspersed with the functions. I think it may work better that way here, to keep the docs for each function with that function, and still have it extractable by pod...

I agree, the original pod docs were taken from the other compat module done by someone else when I consolidated. I will make the changes today and submit a new attachment.

Comment on attachment 144249 [details] Updated DBcompat.pm with better perlpod documentation ># For transaction support >my $intransaction = 0; > ># Temporary tables >my %temptables = (); These will cause mod_perl problems -- see bug 173629. <Avatraxiom> Yeah. Does it have to be "our" or something, instead? 01:56PM <justdave> not necessarily... 01:56PM <justdave> our makes it a global 01:57PM <justdave> in most of those cases what you want to do is restructure the function call so the needed value is passed into it 01:57PM <justdave> or for things like $cgi, retrieve it separately inside each sub. For something like this, we'd probably want to do the last one. Can we store it in $dbi, or something? > description: Outputs SQL syntax for doing obtaining last serial number "doing obtaining" doesn't make sense. :-) > description: Outputs SQL regular expression operator for regex searches in format > suitable for a given database. > Ex: Mysql - SELECT foo FROM bar WHERE foo.column REGEXP 'pattern'; > PostgreSQL - SELECT foo FROM bar WHERE foo.column ~ 'pattern'; > params: $not = negates search pattern (scalar) Do you mean that it's boolean, and if you pass 1 it will negate, or just the existence of anything here will cause it to negate? > description: Outputs SQL syntax for the TO_DAYS Mysql date function based on Database type > params: $column = name of date type column (scalar) > returns: formatted SQL for TO_DAYS function. TO_DAYS($column) for Mysql and just $column for Postgresql. (scalar) These docs aren't accurate for PgSQL anymore. (Also, you probably don't need to specify the implementation in the docs.) Maybe something like "a date field as Julian days". > description: Ouput SQL syntax Typo: Ouput >sub SplitEnumType { You know, the new Pg additions to this function would be a good transition for not using enums in MySQL, too... (Just thinking about bug 17453.) >sub Rand { > return " RAND() " if $db_driver eq 'mysql'; > return " RANDOM() " if $db_driver eq 'Pg'; >} This needs not_implemented for Oracle and Sybase, or equivalent strings. (If there's no actual RAND or RANDOM() function for a DB, could we just use a perl random function and return a string literal...?) >sub Now { > if ($db_driver eq "mysql") { > return "NOW()"; Hrm... I wonder if NOW() should be normalized in format... For the record, I thought the best suggestion from the PgSQL bug for dealing with dates was to store them as ISO strings. Since we're not going to do that, we *could* just go through all of Bugzilla and DateFormat() everything the same way. *sigh* I think that would work. (Of course, that's really a comment for the PgSQL bug.) >sub StartTransaction { > if ($db_driver eq 'mysql') { > # Support to be added when 4.0 is min requirement This is a good case for a db_version variable that can be checked at runtime... If we don't have it already, I suppose that's another bug. > } elsif ($db_driver eq 'Pg') { > Bugzilla->dbh->begin_work if !$intransaction; > $intransaction = 1; I think that we should throw a warning here if we're in a transaction. Bugzilla should never try to StartTransaction() when there's a transaction going, though it might. > } elsif ($db_driver eq 'Pg') { > # Drop any temporary tables > foreach my $table (keys %temptables) { > Bugzilla->dbh->do("DROP TABLE $table"); > delete $temptables{$table}; > } I think bbaetz will have to comment on the accuracy of the mapping -- I'm not entirely qualified to do so. :-) How did you come to decide on that mapping? > foreach my $section (@sections) { > my ($table, $mode, $alias, $as); > if ($section =~ / AS /) { > # Create temporary tables when using an alias > ($table, $as, $alias, $mode) = split(' ', $section); > $temptables{$alias} = 1; > my $query = "CREATE TEMPORARY TABLE $alias AS SELECT * FROM $table"; Won't this bog down a large installation? Hrm, with those considerations, I think that you could post another version and ask bbaetz to review it. He's probably the most qualified, in this situation, I'd think.

(In reply to comment #5) > (From update of attachment 144249 [details]) > ># For transaction support > >my $intransaction = 0; > > > ># Temporary tables > >my %temptables = (); > > These will cause mod_perl problems -- see bug 173629. Actually, it won't, because it's inside a package, which has a distinct namespace, and thus the subs aren't nested. It's only a problem when you have nested subs, which under mod_perl only happens in what would have been the main:: namespace.

>(From update of attachment 144249 [details]) >># For transaction support >>my $intransaction = 0; >> >># Temporary tables >>my %temptables = (); > > These will cause mod_perl problems -- see bug 173629. > > For something like this, we'd probably want to do the last one. Can we store >it in $dbi, or something? > I experimented around and tried the following code to see if it would work. I may be still completely off or maybe not. I added the following to Bugzilla.pm: my %_dbi; $_dbi{intransaction} = 0; $_dbi{temptables} = {}; sub dbi { my $class = shift; # Hash containing runtime dbi settings return \%_dbi; } Then in the relevant places in DBcompat.pm I did something like this: sub EndTransaction { my $dbi = Bugzilla->dbi; # Runtime dbi settings if ($db_driver eq 'mysql') { # Support to be added when 4.0 is min requirement return; } elsif ($db_driver eq 'Pg') { # Drop any temporary tables foreach my $table (keys %{$dbi->{temptables}}) { Bugzilla->dbh->do("DROP TABLE $table"); delete $dbi->{temptables}{$table}; } Bugzilla->dbh->commit if $dbi->{intransaction}; $dbi->{intransaction} = 0; } elsif ($db_driver eq 'Oracle') { Bugzilla->dbh->commit if $dbi->{intransaction}; $dbi->{intransaction} = 0; } elsif ($db_driver eq 'Sybase') { Bugzilla->dbh->commit if $dbi->{intransaction}; $dbi->{intransaction} = 0; } } Do you guys think this is a better solution with respect to mod_perl? >> description: Outputs SQL syntax for doing obtaining last serial number > > "doing obtaining" doesn't make sense. :-) Fixed. >> description: Outputs SQL regular expression operator for regex searches in format >> params: $not = negates search pattern (scalar) > > Do you mean that it's boolean, and if you pass 1 it will negate, or just the >existence of anything here will cause it to negate? > It is meant to be that if $not is true then negate the conditional. So 1 or any string for that matter would make it true. Will word it better. >> description: Outputs SQL syntax for the TO_DAYS Mysql date function based on Database type >> params: $column = name of date type column (scalar) >> returns: formatted SQL for TO_DAYS function. TO_DAYS($column) for Mysql and just $column for Postgresql. (scalar) > > These docs aren't accurate for PgSQL anymore. (Also, you probably don't need >to specify the implementation in the docs.) Maybe something like "a date field >as Julian days". Fixed >> description: Ouput SQL syntax > > Typo: Ouput Fixed. >>sub SplitEnumType { > > You know, the new Pg additions to this function would be a good transition >for not using enums in MySQL, too... (Just thinking about bug 17453.) I just saw where the bug responsible for removing enum data types in the database has been pushed to 2.20 or maybe that was a different one so we may need this a little while longer. >>sub Rand { >> return " RAND() " if $db_driver eq 'mysql'; >> return " RANDOM() " if $db_driver eq 'Pg'; >>} > > This needs not_implemented for Oracle and Sybase, or equivalent strings. (If >there's no actual RAND or RANDOM() function for a DB, could we just use a perl >random function and return a string literal...?) Added code errors for Oracle and Sybase >>sub StartTransaction { >> if ($db_driver eq 'mysql') { >> # Support to be added when 4.0 is min requirement > > This is a good case for a db_version variable that can be checked at >runtime... If we don't have it already, I suppose that's another bug. > >> } elsif ($db_driver eq 'Pg') { >> Bugzilla->dbh->begin_work if !$intransaction; >> $intransaction = 1; > > I think that we should throw a warning here if we're in a transaction. >Bugzilla should never try to StartTransaction() when there's a transaction >going, though it might. You are right, multiple transactions should not happen. But here in the code I gracefully continue if already in one without trying to start another which should also work. There is definitely a better way to do this though. > >> } elsif ($db_driver eq 'Pg') { >> # Drop any temporary tables >> foreach my $table (keys %temptables) { >> Bugzilla->dbh->do("DROP TABLE $table"); >> delete $temptables{$table}; >> } > > I think bbaetz will have to comment on the accuracy of the mapping -- I'm not >entirely qualified to do so. :-) How did you come to decide on that mapping? > This should not hurt anything as temptables are really only needed by Pg for handling table locks with aliases. The %temptables hash keeps a list of the alias names so I can clean up before a transaction is ended. Using a hash makes sure I don't drop the same table twice. >> foreach my $section (@sections) { >> my ($table, $mode, $alias, $as); >> if ($section =~ / AS /) { >> # Create temporary tables when using an alias >> ($table, $as, $alias, $mode) = split(' ', $section); >> ($table, $as, $alias, $mode) = split(' ', $section); >> $temptables{$alias} = 1; >> my $query = "CREATE TEMPORARY TABLE $alias AS SELECT * FROM $table"; > > Won't this bog down a large installation? I can try it for a while in our environment and see how well it does. Someone with more database knowledge can maybe help me to figure out a better way to to table aliases in the lock tables. Maybe do away with aliases somehow?

(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 144249 [details]) > > ># For transaction support > > >my $intransaction = 0; > > > > > ># Temporary tables > > >my %temptables = (); > > > > These will cause mod_perl problems -- see bug 173629. > > Actually, it won't, because it's inside a package, which has a distinct > namespace, and thus the subs aren't nested. It's only a problem when you have > nested subs, which under mod_perl only happens in what would have been the > main:: namespace. Ok, but please take a look at my proposed change mentioned in comment 7 as it might be better since it would allow access in other places instead of just DBcompat.pm. Knowing whether we are in a transaction current for example could be useful in certain scripts.

(In reply to comment #5) > This is a good case for a db_version variable that can be checked at > runtime... If we don't have it already, I suppose that's another bug. Assuming DBD::mysql implements it (it should), the following should work: $db_version = Bugzilla->dbh->get_info( 18 ); # 18 == SQL_DBMS_VER > For the record, I thought the best suggestion from the PgSQL bug for dealing > with dates was to store them as ISO strings. Wow. Thanks. I thought I was just being ignored.

(In reply to comment #9) > (In reply to comment #5) > > This is a good case for a db_version variable that can be checked at > > runtime... If we don't have it already, I suppose that's another bug. > > Assuming DBD::mysql implements it (it should), the following should work: > > $db_version = Bugzilla->dbh->get_info( 18 ); # 18 == SQL_DBMS_VER > > > For the record, I thought the best suggestion from the PgSQL bug for dealing > > with dates was to store them as ISO strings. > > Wow. Thanks. I thought I was just being ignored. I agree with the ISO strings instead of date types since now having worked with Bugzilla with 3 different database, date handling is somewhat of a headache when going from one to another. Using Perl to do all date formatting and then just storing it in the DB would be much more portable. But correct me if I am wrong, wouldn't date arithmetic be much more harder since it could no longer be done in the SQL statements and would have be sorted out after selecting all possible matches?

(In reply to comment #10) > I agree with the ISO strings instead of date types since now having worked with > Bugzilla with 3 different database, date handling is somewhat of a headache when > going from one to another. Using Perl to do all date formatting and then just > storing it in the DB would be much more portable. But correct me if I am wrong, > wouldn't date arithmetic be much more harder since it could no longer be done in > the SQL statements and would have be sorted out after selecting all possible > matches? Yeah, date arithmetic in SQL is harder if you store the dates as ISO 8601 strings, but not impossible unless you want to be completely portable. All DBMSes allow you to convert a string to whatever date/ time data type the DBMS uses and then you go do date arithmetic with that. The conversion function is not portable though and the conversion adds overhead to the arithmetic calculations. Most applications don't need to do much in the way of date arithmetic anyway, so this is acceptable for many DBMS- portable applications. If date arithmetic performance/portability is important, then the other option is to store the date/times as Modified Julian Dates in standard float (8-byte) fields. Since it's a number, date arithmetic is easy, obvious, and fast. All you need are the proper functions to convert date/times to and from MJD format for display, and I've got those. More info on MJD: http://tycho.usno.navy.mil/ mjd.html

Don't forget that we can write unit_timestamp <=> date conversion functions for postgres. That is then transparent from the bugzilla point of view. We need to have something which can trivially do date arithmetic in SQL.

Comment on attachment 144355 [details] [diff] [review] DBcompat.pm (v2) with addition to Bugzilla.pm for dbi runtime settings I'd much rather we just used on impmentation. Due to MySQl's lack of support for SQL functions, thats likely to be MySQl, unfortunately. It makes the raw SQL easier to use and read, too. Stuff which can'tbe done that way should have an 'ansi standard sql' version by default, and a 'mysql' version (or whatever) as an override for databases which aren't standards complient. >Index: Bugzilla.pm >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v >retrieving revision 1.9 >diff -u -r1.9 Bugzilla.pm >--- Bugzilla.pm 27 Nov 2003 01:00:59 -0000 1.9 >+++ Bugzilla.pm 19 Mar 2004 22:16:15 -0000 >@@ -126,6 +126,16 @@ > return $_dbh; > } > >+my %_dbi; >+$_dbi{intransaction} = 0; >+$_dbi{temptables} = {}; >+sub dbi { >+ my $class = shift; >+ >+ # Hash containing runtime dbi settings >+ return \%_dbi; >+} this isn't mod perl safe, unless it gets reset somewhere, because %_dbi won't be cleared each run >Index: Bugzilla/DBcompat.pm >=================================================================== >RCS file: Bugzilla/DBcompat.pm >diff -N Bugzilla/DBcompat.pm >+=head1 SYNOPSIS >+ >+ use Bugzilla::DBcompat; >+ >+ SendSQL("UPDATE bugs SET delta_ts = " . Bugzilla::DBcompat::Now() . " WHERE bug_id = $bugid"); don't use SendSQL as an example - its heavily deprecated. >+=item C<LastKey()> Whats wrong with $dbh->last_insert_id ? >+=item C<RegExp()> This isn't actaully useful, since the format of the regexp is different for mysql vs postgres. CREATE OPERATOR would work on pg, I guess. >+=item C<ToDays()> Using this to search on stuff will break indexes. I'd rather we stored it in the native format. >+=item C<If()> Oracle does let you use case.... I'm not confinved about implementing some of these function for some dbs, but not for others. so does mysql, from 3.23.3. Lets just use it everywhere, since its the standard. >+=item C<SplitEnumType()> Lets just get rid of enums entirely.... >+=item C<Rand()> CREATE FUNCTION >+=item C<DBConnect()> >+ >+ description: Returns connect string for calling DBI->connect depending on database type >+ params: none >+ returns: $dsn = properly formatted DBI connect string (scalar) Since we only connect in one place, and I want to get rid of this module, this can be part of Bugzilla::DBI. >+=item C<DaysDiff()> Nope. Anything which uses this won't end up using indexes on the underlying query, due to the ToDate. Use data arithmetic >+=item C<SecondsDiff()> Ditto. >+=item C<IfNullNumeric()> >+=item C<IfNullString()> Why can't we just always quote these as approprite, using ->quote? (And '$two' is _NOT_ safe....) >+=item C<Length()> Hmm. Where do we actually use this? >+=item C<Limit()> mysql supports the standard LIMIT OFFSET notation from 4.0.6. I'm pretty sure that Oracle does, too. >+=item C<Now()> CREATE FUNCTION for oracle/sybase. >+=item C<Position()> Do we use this? >+=item C<DateFormat()> This is something which should probably be done in perl (or probably even in TT) That way we can do localisation, too. >+=item C<StartTransaction()> $dbh->begin? What does this gain us? >+=item C<EndTransaction()> Ditto. Doesn't PG drop temp tables anyway? >+=item C<LockTables()> This should be passed a real datastructure, not a string to be parsed. >+=item C<UnlockTables()> How does mysql work with locks and transactions?

(In reply to comment #14) > Stuff which can'tbe done that way should have an 'ansi standard sql' version by > default, and a 'mysql' version (or whatever) as an override for databases which > aren't standards complient. I agree. That's basically what this module should do, except in a more modular fashion than writing separate SQL statements throughout Bugzilla for incompatible databases. I do agree, though, that as much SQL should become ANSI-standard as can. (Also, my understanding was that Sybase support would be really difficult without something like this module.) >+=item C<ToDays()> > > Using this to search on stuff will break indexes. I'd rather we stored it in > the native format. I think that would be another bug than this, since this just does exactly what Bugzilla does now. > I'm not confinved about implementing some of these function for some dbs, but > not for others. They'll all get implemented eventually. The "not_implemented" errors are just placeholders, to make it easier for future contributors to patch in cross-db support. > >+=item C<SplitEnumType()> > > Lets just get rid of enums entirely.... I wholeheartedly agree with that. :-) Of course, that would mean fixing bug 17453. This version of this function will make fixing bug 17453 easier, so why don't we keep it until that bug is fixed, and then remove it? >+=item C<Limit()> > > mysql supports the standard LIMIT OFFSET notation from 4.0.6. I'm pretty sure > that Oracle does, too. Bug 204217 (require MySQL 4.0) is targeted for Bugzilla 2.20. If 2.20 requires MySQL 4.0, we can remove or modify this function then. A good thing about this module is that if we need to change some common SQL, it's all right here.

My understanding is that Sybase support is going to be really difficult anyway. Did we ever work out the multiple-simultanious-statements issue? Yes, the ToDays thing is used in a couple of places, but only because MySQL doesn't really have another way of doing it (historically; it may now). Lts not penalise better dbs. > A good thing about this module is that if we need to change some common SQL, > it's all right here. Sure, but its cleaner if we can just use the raw SQL.

(In reply to comment #16) > My understanding is that Sybase support is going to be really difficult anyway. > Did we ever work out the multiple-simultanious-statements issue? I am exploring a number of options with regards to that. I'm confident there will be a mutually agreeable solution. Anyway, this has nothing to do with DBCompat.pm. Feel free to follow up in the Sybase-compatibility bug if you are curious. I would like to voice my support for DBCompat.pm. If I were writing Bugzilla from scratch, this isn't how I would engineer it personally, but I do think it's the best option for Bugzilla right now.

(In reply to comment #14) > (From update of attachment 144355 [details] [diff] [review]) > I'd much rather we just used on impmentation. Due to MySQl's lack of support > for SQL functions, thats likely to be MySQl, unfortunately. > > It makes the raw SQL easier to use and read, too. > > Stuff which can'tbe done that way should have an 'ansi standard sql' version by > default, and a 'mysql' version (or whatever) as an override for databases which > aren't standards complient. So are you suggesting doing something like the following in the code everywhere where we do a DB action? if (AnsiDatabase()) { SendSQL("SOME ANSI COMPLIANT SQL"); } else { SendSQL("NON ANSI COMPLIANT SQL WITH UTILITY FUNCTIONS"); } > >+my %_dbi; > >+$_dbi{intransaction} = 0; > >+$_dbi{temptables} = {}; > >+sub dbi { > >+ my $class = shift; > >+ > >+ # Hash containing runtime dbi settings > >+ return \%_dbi; > >+} > > this isn't mod perl safe, unless it gets reset somewhere, because %_dbi won't > be cleared each run > Removed. Reverted back to old way after discussion with justdave about this in IRC. > >+ use Bugzilla::DBcompat; > >+ > >+ SendSQL("UPDATE bugs SET delta_ts = " . Bugzilla::DBcompat::Now() . " WHERE bug_id = $bugid"); > > don't use SendSQL as an example - its heavily deprecated. > Fixed. > >+=item C<LastKey()> > > Whats wrong with $dbh->last_insert_id ? I have added this to the LastKey function and for some reason I cannot get it to work correctly with Pg. It always returns a null value even when the previous insert is successful. Will look into it some more. The DBI documentation has the following caveats to say about this function and maybe should hold off using it for now: >* For some drivers the value may only available immediately after the insert >statement has executed (e.g., mysql, Informix). Not really a problem as long as we call LastKey() where we currently do now (right after the insert). >* For some drivers the $catalog, $schema, $table, and $field parameters are >required (e.g., Pg), for others they are ignored (e.g., mysql). Not a problem really since I do that now with LastKey() and ignore the params in DBcompat.pm for Mysql. >* Drivers may return an indeterminate value if no insert has been performed yet. I guess this also would not be a problem if we make sure we do an insert prior to calling the LastKey() function. >* For some drivers the value may only be available if placeholders have not >been used (e.g., Sybase, MS SQL). In this case the value returned would be from > the last non-placeholder insert statement. This could be a problem later down the road when we try to incorporate better Sybase support. We are starting to use placeholders in some places I have noticed. >* Some drivers may need driver-specific hints about how to get the value. For >example, being told the name of the database > 'sequence' object that holds the value. Any such hints are passed as >driver-specific attributes in the \%attr parameter. I think they may be talking about Oracle here. >* If no insert has been performed yet, or the last insert failed, then the >value is implementation defined. How can we properly check to make sure that the last insert didnt fail? DBI::Error? > > >+=item C<RegExp()> > > This isn't actaully useful, since the format of the regexp is different for > mysql vs postgres. > > CREATE OPERATOR would work on pg, I guess. And with stored external procedures this may also be possible with Oracle and Sybase but even then wouldnt we still need this type of method for returning the proper format? Since I know Oracle doesnt support regex directly and would require a function to be created such as RexExp(). > >+=item C<If()> > > Oracle does let you use case.... > > I'm not confinved about implementing some of these function for some dbs, but > not for others. > > so does mysql, from 3.23.3. Lets just use it everywhere, since its the > standard. > Removed the If() and put CASE statements in the right places. Only two places really, both in process_bug.cgi > >+=item C<SplitEnumType()> > > Lets just get rid of enums entirely.... Love to. Another bug. > >+=item C<DBConnect()> > >+ > >+ description: Returns connect string for calling DBI->connect depending on database type > >+ params: none > >+ returns: $dsn = properly formatted DBI connect string (scalar) > > Since we only connect in one place, and I want to get rid of this module, this > can be part of Bugzilla::DBI. > I was trying to keep the if () else () stuff out of Bugzilla::DBI for db independence but I moved it back. > >+=item C<Length()> > > Hmm. Where do we actually use this? attachment.cgi and Bugzilla/Attachment.pm are the only places I saw it so I added it in. It seems the reason for this is so we dont have to actually suck in the data portion of the attachment just to run the perl length() function on it for displaying attachment size. > > >+=item C<Limit()> > > mysql supports the standard LIMIT OFFSET notation from 4.0.6. I'm pretty sure > that Oracle does, too. You mean from Mysql 4.0.6+. Is 4.0+ going to be a requirement for 2.18? > >+=item C<Position()> > > Do we use this? Yes. Search.pm primarily. The Mysql specific statements use INSTR instead of POSITION. In the current code. Although Mysql does support POSITION similar to Postgresql and we could just change the SQL to use POSITION instead of using this call but Oracle and Sybase may not. > > >+=item C<DateFormat()> > > This is something which should probably be done in perl (or probably even in > TT) That way we can do localisation, too. Should this be another bug later since it is not a regression from the way we current do dates? But I do agree that we should have one date format for storage in the database and do whatever manipulation that is needed in Perl code after select. Someone in bug 98304 mentioned storing dates in strings but that will make date math interesting. > > >+=item C<StartTransaction()> > > $dbh->begin? What does this gain us? Starts a transaction in Pg which automatically turns of AutoCommit and allows the use of LOCK TABLE statements? > > >+=item C<EndTransaction()> > > Ditto. Doesn't PG drop temp tables anyway? > You are correct now that I dug deeper. Changed. Got rid of the %temptables data structure. > >+=item C<LockTables()> > > This should be passed a real datastructure, not a string to be parsed. > I agree but since this was broken into two bugs separate from 98304, I decided to leave the SQL the same for now. When 98304 lands I will change the necessary LockTable() calls everywhere in the code and also this section.

(In reply to comment #18) > (In reply to comment #14) > > Stuff which can'tbe done that way should have an 'ansi standard sql' version by > > default, and a 'mysql' version (or whatever) as an override for databases which > > aren't standards complient. > > So are you suggesting doing something like the following in the code everywhere > where we do a DB action? > > if (AnsiDatabase()) { > SendSQL("SOME ANSI COMPLIANT SQL"); > } else { > SendSQL("NON ANSI COMPLIANT SQL WITH UTILITY FUNCTIONS"); > } > No, I'm suggesting that we have: sub someCompatFunction() { if ($mysql) { mysql_thing(); } else { do_standard_thing(); } } That means that when adding a new function, you only hafve to add the exceptions > > >+=item C<LastKey()> > > > > Whats wrong with $dbh->last_insert_id ? > > I have added this to the LastKey function and for some reason I cannot get it to > work correctly with Pg. It always returns a null value even when the previous > insert is successful. Will look into it some more. > Hmm. Looking at the code, the DBD drivers don't support it. So patch dBD::Pg, then require the next released version :) > The DBI documentation has the following caveats to say about this function and > maybe should hold off using it for now: > > >* For some drivers the value may only available immediately after the insert > >statement has executed (e.g., mysql, Informix). thats not a restriction on the DBI last_key function, its a limitation of SELECT LAST_INSERT_ID - there is only one. Since thats how mysql works, it shoudn't be a problem. > >* For some drivers the $catalog, $schema, $table, and $field parameters are > >required (e.g., Pg), for others they are ignored (e.g., mysql). > > > Not a problem really since I do that now with LastKey() and ignore the params in > DBcompat.pm for Mysql. Right. > > >* Drivers may return an indeterminate value if no insert has been performed yet. > I guess this also would not be a problem if we make sure we do an insert prior > to calling the LastKey() function. Well, if you connect to a mysqldb, and the first thing you do is select LATS_INSERT_ID, any numer you get won't be really meaningful anyway. Again, not an issue. > >* For some drivers the value may only be available if placeholders have not > >been used (e.g., Sybase, MS SQL). In this case the value returned would be from > > the last non-placeholder insert statement. > This could be a problem later down the road when we try to incorporate better > Sybase support. We are starting to use placeholders in some places I have noticed. Right, which is another reason I don't want to suppot sybase. I don't want to have to uglify our code to support a niche system, with plenty of free alternatives. > >* Some drivers may need driver-specific hints about how to get the value. For > >example, being told the name of the database > > 'sequence' object that holds the value. Any such hints are passed as > >driver-specific attributes in the \%attr parameter. > > I think they may be talking about Oracle here. And postgres? >* If no insert has > been performed yet, or the last insert failed, then the >value is implementation > defined. > > How can we properly check to make sure that the last insert didnt fail? DBI::Error? We should be using RaiseError, and aborting on all errors. > > >+=item C<RegExp()> > > > > This isn't actaully useful, since the format of the regexp is different for > > mysql vs postgres. > > > > CREATE OPERATOR would work on pg, I guess. > > And with stored external procedures this may also be possible with Oracle and > Sybase but even then wouldnt we still need this type of method for returning the > proper format? Since I know Oracle doesnt support regex directly and would > require a function to be created such as RexExp(). Possibly. I think you can create an operator in postgres, though. I'm not too fussed about this one. > > >+=item C<DBConnect()> > > Since we only connect in one place, and I want to get rid of this module, this > > can be part of Bugzilla::DBI. > I was trying to keep the if () else () stuff out of Bugzilla::DBI for db > independence but I moved it back. I'm willing to be convinced. In fact, why can't this entire module be part of Bugzilla::DB? We can call it as methods off the DBH, if we do intheritance right, then do method overididing for mysql/Pg/etc where required. That handles the 'nonstandard' bit I mentioned above. Thoughts? > > >+=item C<Length()> > > > > Hmm. Where do we actually use this? > > attachment.cgi and Bugzilla/Attachment.pm are the only places I saw it so I > added it in. It seems the reason for this is so we dont have to actually suck in > the data portion of the attachment just to run the perl length() function on it > for displaying attachment size. Hmm. Is it worth abstracting the attachment stuff a bit more into a large object style API? That coudl also handle the 'compress-it-before-putting-it-in-the DB' style stuff, plus the binary conversions some dbs need? I don't think we need the full LO support - anyoine who sticks gigabyte attachments into bugzilla is going to have other problems anyway. > > >+=item C<Limit()> > > > > mysql supports the standard LIMIT OFFSET notation from 4.0.6. I'm pretty sure > > that Oracle does, too. > > You mean from Mysql 4.0.6+. Is 4.0+ going to be a requirement for 2.18? Not for 2.18, but postgres support isn't going to happen for 2.18 either. > > >+=item C<Position()> > > > > Do we use this? > > Yes. Search.pm primarily. The Mysql specific statements use INSTR instead of > POSITION. In the current code. Although Mysql does support POSITION similar to > Postgresql and we could just change the SQL to use POSITION instead of using > this call but Oracle and Sybase may not. > Hmm. Can we find out? > > >+=item C<DateFormat()> > > > > This is something which should probably be done in perl (or probably even in > > TT) That way we can do localisation, too. > > Should this be another bug later since it is not a regression from the way > we current do dates? But I do agree that we should have one date format for > storage in the database and do whatever manipulation that is needed in Perl > code after select. Someone in bug 98304 mentioned storing dates in strings > but that will make date math interesting. Uf we're going to change all the date users, I'd prefer to only do so once. > > >+=item C<StartTransaction()> > > > > $dbh->begin? What does this gain us? > > Starts a transaction in Pg which automatically turns of AutoCommit and allows > the use of LOCK TABLE statements? What does this wrapper gain us over $dbh->begin, is what I meant. > > >+=item C<LockTables()> > > > > This should be passed a real datastructure, not a string to be parsed. > > > > I agree but since this was broken into two bugs separate from 98304, I decided > to leave the SQL the same for now. When 98304 lands I will change the necessary > LockTable() calls everywhere in the code and also this section. OK. I'm not sure that this can be reliably abstacted as is, but we can try it and see, I guess.

(In reply to comment #20) [...] > No, I'm suggesting that we have: > > sub someCompatFunction() { > if ($mysql) { > mysql_thing(); > } else { > do_standard_thing(); > } > } How about we rely on the framework that DBI provides. The switching seems like something that we should do through inheritance. The latest versions of DBI have DBIx::AnyDBD style subclassing support: http://groups.google.com/groups?q=tim+bunce+DBIx::AnyDBD&hl=en&lr=&ie=UTF-8&selm=20030930172652.GJ42989%40dansat.data-plan.com&rnum=4 (No documentation yet. It is planned for DBI 2) [... last_insert_id ...] > Hmm. Looking at the code, the DBD drivers don't support it. So patch dBD::Pg, > then require the next released version :) It looks like DBD::mysql is getting fixed: http://www.mail-archive.com/dbi-dev@perl.org/msg03050.html > >* If no insert has > > been performed yet, or the last insert failed, then the >value is implementation > > defined. > > > > How can we properly check to make sure that the last insert didnt fail? > DBI::Error? > > We should be using RaiseError, and aborting on all errors. We will need this if we want to support transactions. (Yahoo's installation of bugzilla seems to bog down during peak use because of the table locks. I am looking to switch at least some of the tables to use transactions (and InnoDB tables).) > > > >+=item C<DBConnect()> > > > > Since we only connect in one place, and I want to get rid of this module, this > > > can be part of Bugzilla::DBI. > > > I was trying to keep the if () else () stuff out of Bugzilla::DBI for db > > independence but I moved it back. > > I'm willing to be convinced. In fact, why can't this entire module be part of > Bugzilla::DB? We can call it as methods off the DBH, if we do intheritance > right, then do method overididing for mysql/Pg/etc where required. That handles > the 'nonstandard' bit I mentioned above. Thoughts? This is what I have wondered the last couple days as I have followed this thread. There are many other existing frameworks for this type of database abstraction: DBIx::SQLEngine, DBIx::AnyDBD, DBIx::Rosetta, DBIx::Abstract, DBIx::DWIW, and Class::DBI (this last one is a very different approach). I briefly looked that the first two, and the seemed like something we could leverage. DBIx::SQLEngine looks very clean and handy. Also, as I mentioned earlier, the latest DBI has subclassing support like DBIx::AnyDBD. The feature just is not documented.

> How about we rely on the framework that DBI provides. The switching seems like > something that we should do through inheritance. Right - I suggested that later on. How does DBIx::AnyDBD differ from the DBI inheritance stuff? Also, the DBI inheritance is documented. What changes were made? Also, thats slightly different to what I was proposing. I wanted the functions to return SQL, not do 'stuff'. The biggest problem I have with those type of modules is that you end up abstracting the SQL away so much that queries involving multiple tables become a real pain. I haven't seen one of these which can handle subselects usefully, either. (This is possibly beacuse mysql support is usually the first thing which goes in...) I'm willing to be convinced, though. (also, if you have a large number of bugs and are moving to innodb, you'll want to benchmark it first. I've seen massive slowdowns on a dummy bugzilla instance with 50,000 bugs or so, when doing a join or two. May be because its incredibly RAM hungry, but I wasn't swapping that much)

> We will need this if we want to support transactions. I assume you mean that we will be needed RaiseError? We already have it set, actually.

I'm not convinced yet of any reason this will be any benefit to be part of 2.18 without any of the rest of the code actually using it yet. That said, I probably won't stop if from being checked in if it happens to be ready by then, but this is probably better done on the 2.19 branch anyway.

I was just looking at the Limit sub. Instead of just returning the LIMIT sql code, if the sub was passed the full SELECT statement, it could then modify it and return back the SELECT statement with the LIMIT included. There would then be no need to have special cases for Oracle or Sybase. sub LimitSelect { my ($query,$limit,$offset) = @_; if ($db_driver eq "mysql") { if ($offset) { return "$query LIMIT $offset,$limit"; } else { return "$query LIMIT $limit"; } } elsif ($db_driver eq "Oracle") { return "SELECT * FROM ($query) WHERE ROWNUM <= $limit"; } elsif ($db_driver eq "Pg") { if ($offset) { return "$query LIMIT $limit OFFSET $offset"; } else { return "$query LIMIT $limit"; } } elsif ($db_driver eq "Sybase") { #NOTE: May need to add a cr or lf between the set rowcounts and the query return "set rowcount $limit " . "$query" . "set rowcount 0"; } }

(In reply to comment #25) In that case, it should probably be a modification of SendSQL or whatever that function is that we use, now.

(In reply to comment #26) > (In reply to comment #25) > In that case, it should probably be a modification of SendSQL or whatever that > function is that we use, now. No, I disagree. Comment #25 is the way to go. Usage should be more like this: SendSQL(LimitSelect($sql,$limit,$offset)).

(In reply to comment #25) > I was just looking at the Limit sub. Instead of just returning the LIMIT sql > code, if the sub was passed the full SELECT statement, it could then modify it > and return back the SELECT statement with the LIMIT included. > > There would then be no need to have special cases for Oracle or Sybase. > > sub LimitSelect { > my ($query,$limit,$offset) = @_; > if ($db_driver eq "mysql") { > if ($offset) { > return "$query LIMIT $offset,$limit"; > } else { > return "$query LIMIT $limit"; > } > } > elsif ($db_driver eq "Oracle") { > return "SELECT * FROM ($query) WHERE ROWNUM <= $limit"; > } > elsif ($db_driver eq "Pg") { > if ($offset) { > return "$query LIMIT $limit OFFSET $offset"; > } else { > return "$query LIMIT $limit"; > } > } > elsif ($db_driver eq "Sybase") { > #NOTE: May need to add a cr or lf between the set rowcounts and the query > return "set rowcount $limit " . "$query" . "set rowcount 0"; > } > } > This looks good for LIMIT but would not be as useful for other cases where the DB specific SQL is located in other parts of the SQL statement. LIMIT usually occurs on the end of the SQL so this would work. But for things like DateFormat() and others they would need to be able to be placed anywhere. I personally like the way I have it currently since it makes all of the DB functions work consistently.

(In reply to comment #25) Could also add (if required) elsif ($db_driver eq "DB2") { return "$query FETCH FIRST $limit ROWS ONLY"; } elsif ($db_driver eq "MSSQL") { if (substr($query,1,7 eq "SELECT ") { return "SELECT TOP $limit " . substr($query,8,99999); } else { return $query; } }

Some additional Oracle suppoprt for Attachment DBcompat.pm (v3): sub IfNullNumeric { ... elsif ($db_driver eq "Oracle") { # Could use NVL if needed to support Oracle 8i return "COALESCE($one,$two)"; } sub IfNullString { ... elsif ($db_driver eq "Oracle") { # Could use NVL if needed to support Oracle 8i return "COALESCE($one,$two)"; } sub Length { ... elsif ($db_driver eq "Oracle") { return "LENGTH($str)"; } sub Position { ... elsif ($db_driver eq "Oracle") { return "INSTR($str1,$str2)"; }

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004. Anything flagged enhancement that hasn't already landed is being pushed out. If this bug is otherwise ready to land, we'll handle it on a case-by-case basis, please set the blocking2.20 flag to '?' if you think it qualifies.

I would very much like to land this for 2.20, and here's why: As time goes on, more and more DB-dependent code will be added to Bugzilla. This will make Oracle/PostgreSQL/*SQL support much more difficult in each release, instead of easier. However, if we check this in now, and start using it incrementally for all future builds, and patch it as needed in individual bugs, then we'll be moving closer and closer to DB independence, instead of moving further and further away.

I also think this should be in as soon as possible. I put a bit of work into it and here is the result: I tried to incorporate all comments, and I removed all code not esential for postgres, including support for sybase and oracle. Sorry guys :-). Main reasons are two: the support was incomplete and I don't have access to these DBs to finish and test it; the code is now simpler and easier to review. Support for other DBs should be simple to add as soon as this file lands. I will also submit the patch against BZ to make use of this file. I will leave up to the reviewer if just one file or both gets review and land, but I think it needs to be here to better understand the context.

Comment on attachment 162690 [details] [diff] [review] DBcompat.pm (v4) >+ description: Obtainins the last serial number usually from a previous INSERT. >+ For Mysql it assumes that this is called directly following >+ the relevant INSERT, shouldn't matter for PostgreSQL since you >+ specify the table and column of the proper sequence. >+ For some drivers the $table and $column parameters are required >+ (e.g., Pg), for others they are ignored (e.g., mysql). A description is usually an interface definition, not a description of an implementation. So, we should just say how the function is *supposed* to be used (immediately following the previous insert, must include table & column name) and not inform people that they could get away with something less on PgSQL that wouldn't be portable to MySQL or vice-versa. >+ $column = name of column containing serial data type (scalar) Having this parameter is deceptive, since it won't take effect for MySQL. Is there any >+ if ($db_driver eq 'mysql') { >+ $query = "SELECT LAST_INSERT_ID()"; >+ } Didn't we discuss at some time in the past that this should work like the other functions, and just create SQL instead of returning a value? >+ elsif ($db_driver eq 'Pg') { >+ my $seq = $table . "_" . $column . "_seq"; >+ $query = "SELECT CURRVAL('$seq')"; >+ } Like bbaetz said above, let's just make this "else", since I think that's an ANSI-standard function. Also do that everywhere else where PG uses the ANSI standard, which is everywhere that I know of, though leave it as "if pg" if you're not certain (since that's safer). >+=item C<RegExp()> Looks fine to me... but is the RegEx syntax the same in Pg and MySQL? I seem to recall this possibly being a problem...? But I think it's not, otherwise dkl would have known already. >+=item C<Limit()> Looks good -- same comment about "else" applies (since it applies to all the functions). >+ if ($db_driver eq 'mysql') { >+ # Support to be added when 4.0 is min requirement >+ } Um, if I tell Bugzilla to Rollback, it's usually because it's REALLY IMPORTANT that it does so. This either needs to throw an error, or tell DBI not to commit the transaction, or something. Also, does StartTransaction turn off AutoCommit? (Is that necessary?) I think everything else certainly looks good enough to check in, and I'd personally very much like to start using this. I'm not certain I'm qualified by bugzilla.org to give this the "-", though.

> Is there any *cough* Is there any way to get other databases to act the way that MySQL does, automatically?

(In reply to comment #35) > (From update of attachment 162690 [details] [diff] [review]) > >+ description: Obtainins the last serial number usually from a previous INSERT. > >+ For Mysql it assumes that this is called directly following > >+ the relevant INSERT, shouldn't matter for PostgreSQL since you > >+ specify the table and column of the proper sequence. > >+ For some drivers the $table and $column parameters are required > >+ (e.g., Pg), for others they are ignored (e.g., mysql). > > A description is usually an interface definition, not a description of an > implementation. So, we should just say how the function is *supposed* to be > used (immediately following the previous insert, must include table & column > name) and not inform people that they could get away with something less on > PgSQL that wouldn't be portable to MySQL or vice-versa. Fixed. > > >+ $column = name of column containing serial data type (scalar) > > Having this parameter is deceptive, since it won't take effect for MySQL. Is > there any way to get other databases to act the way that MySQL does, > automatically? Not any I know of, but if there is any, I would be happy to use it. > > >+ if ($db_driver eq 'mysql') { > >+ $query = "SELECT LAST_INSERT_ID()"; > >+ } > > Didn't we discuss at some time in the past that this should work like the > other functions, and just create SQL instead of returning a value? Fixed. > > >+ elsif ($db_driver eq 'Pg') { > >+ my $seq = $table . "_" . $column . "_seq"; > >+ $query = "SELECT CURRVAL('$seq')"; > >+ } > > Like bbaetz said above, let's just make this "else", since I think that's an > ANSI-standard function. Also do that everywhere else where PG uses the ANSI > standard, which is everywhere that I know of, though leave it as "if pg" if > you're not certain (since that's safer). I checked all the SQL statements in this module, and all seems to be extensions of either MySQL or PgSQL. So I don't think it would be safe. Let's wait for one or two more supported DBs, we can always easily revise this, it's not affecting module interface. > > >+=item C<RegExp()> > > Looks fine to me... but is the RegEx syntax the same in Pg and MySQL? I seem > to recall this possibly being a problem...? But I think it's not, otherwise dkl > would have known already. Both are claiming POSIX regular expression syntax, so let's hope :-). > > >+=item C<Limit()> > > Looks good -- same comment about "else" applies (since it applies to all the > functions). As above, it's an extension. > > > >+ if ($db_driver eq 'mysql') { > >+ # Support to be added when 4.0 is min requirement > >+ } > > Um, if I tell Bugzilla to Rollback, it's usually because it's REALLY > IMPORTANT that it does so. This either needs to throw an error, or tell DBI not > to commit the transaction, or something. This gets called before returning error, so error gets printed at the end. The behaviour is what's already in Bugzilla. Is it right? I think it's not, as it can leave DB in inconsistent state on error. But it is definitely out of the scope of this bug. And it is not easy to fix without transactions. So I think the best is to wait for Bugzilla to require MySQL version supporting transactions, and then implement it here. It should then be enough to add the code here, it should work without any changes to any other file. > > Also, does StartTransaction turn off AutoCommit? (Is that necessary?) Yes, it does. dbh->begin_work automatically turns autocommit off. > > I think everything else certainly looks good enough to check in, and I'd > personally very much like to start using this. > Glad to hear that :-). It would be great to get it finally in.

Comment on attachment 164081 [details] [diff] [review] BZ changes to make use of DBcompat.pm (v2) >Index: attachment.cgi >- SendSQL("LOCK TABLES attachments WRITE , flags WRITE , " . >- "flagtypes READ , fielddefs READ , bugs_activity WRITE, " . >- "flaginclusions AS i READ, flagexclusions AS e READ, " . >+ Bugzilla::DB::LockTables('attachments WRITE', 'flags WRITE', >+ 'flagtypes READ', 'fielddefs READ', >+ 'bugs_activity WRITE', 'flaginclusions AS i READ', >+ 'flagexclusions AS e READ', > # cc, bug_group_map, user_group_map, and groups are in here so we > # can check the permissions of flag requestees and email addresses > # on the flag type cc: lists via the CanSeeBug >@@ -1077,9 +1078,10 @@ > # Bugzilla::User needs to rederive groups. profiles and > # user_group_map would be READ locks instead of WRITE locks if it > # weren't for derive_groups, which needs to write to those tables. >- "bugs READ, profiles WRITE, " . >- "cc READ, bug_group_map READ, user_group_map WRITE, " . >- "group_group_map READ, groups READ"); >+ 'bugs READ', 'profiles WRITE', >+ 'cc READ', 'bug_group_map READ', >+ 'user_group_map WRITE', >+ 'group_group_map READ', 'groups READ'); > Should the LockTables() routine also just return SQL and not execute the command itself in keeping with the other functions? So instead use $dbh->do(Bugzilla::DB::LockTables('attachments WRITE', etc))? >Index: buglist.cgi >- SendSQL("LOCK TABLES namedqueries WRITE"); >+ Bugzilla::DB::LockTables('namedqueries WRITE'); > Same. As well as lots of other places.

Comment on attachment 164081 [details] [diff] [review] BZ changes to make use of DBcompat.pm (v2) >Index: whine.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/whine.pl,v >retrieving revision 1.2 >diff -u -r1.2 whine.pl >--- whine.pl 17 Aug 2004 14:51:04 -0000 1.2 >+++ whine.pl 31 Oct 2004 10:15:03 -0000 >@@ -68,7 +68,7 @@ > "LEFT JOIN whine_events " . > " ON whine_events.id = whine_schedules.eventid " . > "WHERE run_next <= NOW() " . >- "ORDER BY run_next LIMIT 1" >+ "ORDER BY run_next" . Bugzilla::DB::Limit(1); > ); Remove the extra semicolon after the Bugzilla::DB::Limit(1).

Comment on attachment 164080 [details] [diff] [review] DBcompat.pm (v5) >+ >+sub RollbackTransaction { >+ if (!$intransaction) { >+ ThrowCodeError("not_in_transaction"); >+ } >+ >+ if ($db_driver eq 'mysql') { >+ # Support to be added when 4.0 is min requirement >+ } >+ elsif ($db_driver eq 'Pg') { >+ Bugzilla->dbh->rollback; >+ } >+ $intransaction = 0; >+} >+ Nice. >+=item C<LockTables()> >+ >+ description: Starts a transaction (if supported) and performs a table lock >+ operation on specified tables. >+ params: @tables = array containing names of tables to lock in MySQL >+ notation (ex. 'bugs AS bugs2 READ', 'logincookies WRITE') >+ returns: none >+ >+=cut >+ >+sub LockTables { >+ my (@tables) = @_; >+ >+ # Begin Transaction >+ StartTransaction(); >+ if ($db_driver eq 'mysql') { >+ my $query = 'LOCK TABLE ' . join(', ', @tables); >+ Bugzilla->dbh->do($query); >+ } >+ elsif ($db_driver eq 'Pg') { >+ my @read_tables; >+ my @write_tables; >+ foreach my $table (@tables) { >+ $table =~ /^(\w+).*(READ|WRITE)$/i; >+ my $table_name = $1; >+ if ($2 =~ /READ/i) { >+ push(@read_tables, $table_name); >+ } >+ else { >+ push(@write_tables, $table_name); >+ } >+ } >+ Bugzilla->dbh->do('LOCK TABLE ' . join(', ', @read_tables) . >+ ' IN ACCESS SHARE MODE') if @read_tables; >+ Bugzilla->dbh->do('LOCK TABLE ' . join(', ', @write_tables) . >+ ' IN EXCLUSIVE MODE') if @write_tables; >+ } >+ return 1; >+} This will not work on PostgreSQL the way it is currently implemented since PostgreSQL doesn't handle the 'table1 AS table2 READ' scenario without creating temporary tables.

(In reply to comment #36) > > Is there any > > *cough* Is there any way to get other databases to act the way that MySQL does, > automatically? Stored procedures and custom operators could help some in PostgreSQL. There was a couple places where it would be difficult though but can't think of them off the top of my head.

(In reply to comment #35) > >+=item C<RegExp()> > > Looks fine to me... but is the RegEx syntax the same in Pg and MySQL? I seem > to recall this possibly being a problem...? But I think it's not, otherwise dkl > would have known already. > The syntax is different. For MySQL it is: SELECT foo FROM bar WHERE foo REGEXP '$regexp'; and SELECT foo FROM bar WHERE foo NOT REGEXP '$regexp'; and PostgreSQL goes something like: SELECT foo FROM bar WHERE foo ~ '$regexp'; and SELECT foo FROM bar WHERE foo !~ '$regexp';

(In reply to comment #40) > Should the LockTables() routine also just return SQL and not execute the > command itself in keeping with the other functions? So instead use > $dbh->do(Bugzilla::DB::LockTables('attachments WRITE', etc))? > I was thinking about that, but I would prefer it stay as it is. The behaviour of these functions is different, it not only returns single SQL command, but it also start/stop transaction, there may be multiple SQL commands to execute (pg locking) etc. If you insist on it, I think it can be done, but I think it would make it harder to use, not simpler.

(In reply to comment #41) > (From update of attachment 164081 [details] [diff] [review]) > >Index: whine.pl > >=================================================================== > >RCS file: /cvsroot/mozilla/webtools/bugzilla/whine.pl,v > >retrieving revision 1.2 > >diff -u -r1.2 whine.pl > >--- whine.pl 17 Aug 2004 14:51:04 -0000 1.2 > >+++ whine.pl 31 Oct 2004 10:15:03 -0000 > >@@ -68,7 +68,7 @@ > > "LEFT JOIN whine_events " . > > " ON whine_events.id = whine_schedules.eventid " . > > "WHERE run_next <= NOW() " . > >- "ORDER BY run_next LIMIT 1" > >+ "ORDER BY run_next" . Bugzilla::DB::Limit(1); > > ); > > Remove the extra semicolon after the Bugzilla::DB::Limit(1). > Thanks, fixed.

(In reply to comment #42) > This will not work on PostgreSQL the way it is currently implemented since > PostgreSQL doesn't handle the 'table1 AS table2 READ' scenario without creating > temporary tables. > Bugger. I had the code to remove aliases for pg there, I must have lost it somehow on the way... Thanks, fixed.

Fixed all reported problems. I have also put the function Interval() back, as the workaround I was using was not working correctly.

Done more testing, which revealed a few more bugs. Fixed these and all reported bugs.

a few comments.... It seems a little clunky to use Bugzilla::DB::Function() everywhere. Is there a more compact format that would work? Perhaps something like... my $db = new Bugzilla::DB; $db->function() Maybe it could even be part of the obhject returned by Bugzilla->dbh so you can use... $sth = $dbh->prepare($dbh->lock_tables("bugs" => "READ", "profiles" => WRITE")); Also, is there any compike-time differrence between interspersing the perldoc stuff (as the new module does) or sticking it after an __END__ line ???

(In reply to comment #50) > a few comments.... > > It seems a little clunky to use Bugzilla::DB::Function() everywhere. Is there a > more compact format that would work? Perhaps something like... > > my $db = new Bugzilla::DB; > > $db->function() > > Maybe it could even be part of the obhject returned by Bugzilla->dbh so you can > use... > > $sth = $dbh->prepare($dbh->lock_tables("bugs" => "READ", "profiles" => WRITE")); I do not like the idea of making it part of the Bugzilla->dbh object, as it is separate module, not a layer on top of dbh or part of dbh. And I vaguely remember there was some opposition to make it part of dbh or wrapper (that's why we are returning strings and not doing the execute call directly in most cases) with some good reasons. The first way may make all the calls shorter, but I am not sure it makes it clearer. When you see $db->function() somewhere in the code, you don't know if you are calling method on initialized object, or just a function without side effects. Bugzilla::DB::function() makes it clear. We can do almost anything here, so it's probably more up to justdave or someone else with the "Big Picture (tm)" in mind to decide, what is best for Bugzilla. > > Also, is there any compike-time differrence between interspersing the perldoc > stuff (as the new module does) or sticking it after an __END__ line ??? > Maybe marginally longer parsing time? Dunno, but I doubt there is any significant difference. And I really like it this way, as it is much easier to read. Maybe we should adopt it as a global policy for all Bugzilla modules?

Simplified unlocking of tables on error.

Simplified unlocking of tables on error, fixed quoting in one LOCK preventing perl variable expansion.

Oooops, wrong file :-).

I had a flash yesterday night about how this should work: Bugzilla::DB::MySQL Bugzilla::DB::Pg Those are separate classes, we just instantiate the correct one in Bugzilla.pm by reading the db_driver from Bugzilla::Config or localconfig or something like that. Just like other things, we probably just pass the ->new() method an argument that says what DB we're using. Then we don't have to mess with the db_driver global var or anything like that. Also, that lets people with separate DB knowledge work on the separate files individually, and plug in a new database much more easily. Bugzilla::DB should (ideally) implement all of the methods that need to be implemented, but in Bugzilla::DB they should just throw an error. They don't work unless overridden by the subclass (if that's possible in perl).

Oh, actually, what we should do is put ANSI-standard functions in Bugzilla::DB, and then override them as necessary. (Also, reassigning to current patch author.)

(In reply to comment #55) > I had a flash yesterday night about how this should work: > > Bugzilla::DB::MySQL > Bugzilla::DB::Pg > > Those are separate classes, we just instantiate the correct one in Bugzilla.pm > by reading the db_driver from Bugzilla::Config or localconfig or something like > that. Just like other things, we probably just pass the ->new() method an > argument that says what DB we're using. > > Then we don't have to mess with the db_driver global var or anything like that. > > Also, that lets people with separate DB knowledge work on the separate files > individually, and plug in a new database much more easily. > > Bugzilla::DB should (ideally) implement all of the methods that need to be > implemented, but in Bugzilla::DB they should just throw an error. They don't > work unless overridden by the subclass (if that's possible in perl). And also have checksetup.pl use these modules to call appropriate methods to create tables, add/remove/rename fields, etc. in a generic fashion and whichever module is used will know how to accomplish the tasks correctly.

(In reply to comment #57) > And also have checksetup.pl use these modules to call appropriate methods to > create tables, add/remove/rename fields, etc. in a generic fashion and whichever > module is used will know how to accomplish the tasks correctly. Absolutely. :-) Also see bug 277617. :-)

(In reply to comment #55) > I had a flash yesterday night about how this should work: > > Bugzilla::DB::MySQL > Bugzilla::DB::Pg > > Those are separate classes, we just instantiate the correct one in Bugzilla.pm > by reading the db_driver from Bugzilla::Config or localconfig or something like > that. Just like other things, we probably just pass the ->new() method an > argument that says what DB we're using. > > Then we don't have to mess with the db_driver global var or anything like that. > > Also, that lets people with separate DB knowledge work on the separate files > individually, and plug in a new database much more easily. > > Bugzilla::DB should (ideally) implement all of the methods that need to be > implemented, but in Bugzilla::DB they should just throw an error. They don't > work unless overridden by the subclass (if that's possible in perl). Yep, I really like this approach, and if there is a consensus that this is the way to go, I will rework DBcompat.pm patch this way. It's just that this idea was here before (yours as well, I think :-), https://bugzilla.mozilla.org/show_bug.cgi?id=98304#c73), and I am not sure if it was rejected or just fell betwen the cracks.

(In reply to comment #59) > Yep, I really like this approach, and if there is a consensus that this is the > way to go, I will rework DBcompat.pm patch this way. There's consensus. You, dkl, and I are the "PgSQL developers," pretty much. We all agree. :-) > It's just that this idea was here before (yours as well, I think :-), > https://bugzilla.mozilla.org/show_bug.cgi?id=98304#c73), and I am not sure if it > was rejected or just fell betwen the cracks. I think it just fell between the cracks. That bug was attempting to deal with too much.

(In reply to comment #60) > (In reply to comment #59) > > Yep, I really like this approach, and if there is a consensus that this is the > > way to go, I will rework DBcompat.pm patch this way. > > There's consensus. You, dkl, and I are the "PgSQL developers," pretty much. We > all agree. :-) > Excellent, I will start looking into it. Which version of ANSI-standard we should choose for Bugzilla::DB? The only reference I found was for SQL-92 (http://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt), I suppose it should be enough, at least for now? It's probably quite unofficial, as standards are usually paid, but as it is available for a while now (I found a reference to it in a two years old newsgroup post), can someone add the reference to it to SQL References on http://www.bugzilla.org/developers/?

I think we should use SQL-99. That's what MySQL and PostgreSQL strive to be compatible with. I believe that some of these documents contain the technical details of SQL-99: http://www.ncb.ernet.in/education/modules/dbms/SQL99/ Here's a validator for SQL-99: http://developer.mimer.com/validator/parser99/index.tml I think the SQL-99 standard is about 2000 pages. I think that our policy can be, "When in Doubt, read the PostgreSQL manual. They usually say if their syntax is ANSI-standard or not." http://www.postgresql.org/docs/7.4/interactive/index.html

> There's consensus. You, dkl, and I are the "PgSQL developers," pretty much. We > all agree. :-) The only possible problem is we are all developers, I haven't heard anything from any official reviewer, and without a review, it's going to bitrott again :-(. > Bugzilla::DB::MySQL > Bugzilla::DB::Pg > > Those are separate classes, we just instantiate the correct one in Bugzilla.pm > by reading the db_driver from Bugzilla::Config or localconfig or something like > that. Just like other things, we probably just pass the ->new() method an > argument that says what DB we're using. > > Then we don't have to mess with the db_driver global var or anything like that. > Still not clear on the creation part. If you want to do it as a class, then you either add another global variable (instantiated class, which needs to be available everywhere you access DB), or you still need db_driver global to create a new instance for every SQL call (which seems to be a waste of CPU, instantiating a class for every SQL...). So far the best approach I can think of is keep all in modules similar to DBcompat.pm today, and at the begining of Bugzilla.pm "use" (import) generic Bugzilla::DB and then the correct one, based on db_driver value. The later one will overrinde (is it possible in perl?) all functions, which are not ANSI compatible. It's not OO, but we don't need OO stuff like encapsulation here, it's just DB API, right?

Comment on attachment 168291 [details] [diff] [review] DBcompat.pm (v7) Actually, I am a reviewer. (That happened recently; it's not on the Reviewers page yet.) The only place that we need to know the database type is in Bugzilla->dbh, because that's the centralized place in Bugzilla for all DB access. I guess that for now it will have to be a localconfig variable that ends up in the Bugzilla::Config namespace. So just create the correct Bugzilla->dbh somehow based on db_driver.

Oh, I see the problem. We also need to know the db_driver to know which DB to import for when we use the SQL-replacement functions. OK, so then Bugzilla::DB just needs to have some internal code that picks the correct DB type based on db_driver. Although the better thing to do would be one of these two: (1) Make DB into a real object of some kind (although it would be a singleton -- that is, an object that is only instantiated once in the entire application). (2) Attach an object to $dbh that has the correct functions in it.

(In reply to comment #65) > Oh, I see the problem. We also need to know the db_driver to know which DB to > import for when we use the SQL-replacement functions. > > OK, so then Bugzilla::DB just needs to have some internal code that picks the > correct DB type based on db_driver. > > Although the better thing to do would be one of these two: > > (1) Make DB into a real object of some kind (although it would be a singleton -- > that is, an object that is only instantiated once in the entire application). > > (2) Attach an object to $dbh that has the correct functions in it. And what about inheriting the new DBcompat object from DBI, and returning it from connect instead of dbh? Then you can do something like: $dbh = Bugzilla->dbh; $dbh->prepare("SELECT * FROM bugs " . $dbh->limit(50)); It's in the same place as all other DB stuff, always handy when doing DB access... The only disadvantage I can see is that we need to pick method names carefully, not clashing with something existing. But that can be easily prevented by some prefix, eg. $dbh->bz_limit(50), or using different naming conventions, eg. $dbh->Limit(50) (which would make it a bit more consistent with other bugzilla code anyway, although the interface is then a bit inconsistent).

I think that sounds like a fine idea, as long as they can stay in the Bugzilla/DB directory with that structure. We could prefix them either with bz_ or with sql_. You might also want to look at other modules that do something similar in CPAN and see how they work.

Well, here is a first preview of the new OO architecture for database compatibility layer I am just working on. It is not complete, only module for MySQL is currently implemented, documentation is not finished, the code needs a lot of polishing and it has got only very limited testing. So I am not asking for review, but I still want feedback. I would really appreciate if someone could take a look at the patch and add any comments or ideas, while it's still work in progress. The most interesting stuff is the DBmysql.pm module at the start of the patch (a new file with MySQL specific stuff), and DB.pm, which has been changed to a full blown class derived from DBI::db and is serving as a base class for DBmysql (and DBpostgresql and others in the future). In particular, I would like to know if you think it's better to include all the 'abstract' methods (as it is currently in the patch, they are just calling die()) which are supposed to be implemented by the derived class, or if it just makes the code harder to read. I think it may be good at least for documentation purposes... Also, is it ok to just call die() in these 'abstract' methods, or should I go for ThrowCodeError()? These error messages should be seen only if you start playing with the code, never by the end user, but I don't know what's the preffered way of error handling. Any other ideas how to make this beast better? :-)

Hrm. Why DBmysql instead of DB::mysql or DB::MySQL? Also, I think that instead of implementing abstract functions in the base class, we should implement "cross-DB" functions in the base class, and then only have to override them if we need DB-specific versions.

(In reply to comment #69) > Hrm. Why DBmysql instead of DB::mysql or DB::MySQL? Too lazy to create new directory for that? ;-). I will change that for the new version... > > Also, I think that instead of implementing abstract functions in the base class, > we should implement "cross-DB" functions in the base class, and then only have > to override them if we need DB-specific versions. What do you mean by "cross-DB" function? We were talking before about puting all ANSI compliant SQL to the base class, is that what you mean? Unfortunatelly, AFAICT not a single function we are implementing is ANSI (maybe that's the reason every DB is doing it in a different way), so all of them are "abstract". All except transaction support, which is done using DBI, so it should be the same for all DBs. I am just not sure what it will do on older MySQLs which do not have transactions. If the call is ignored, it's ok, if it spits error, we need to check MySQL version before calling. I plan to look at that later.

Well, it's finished. This patch contains only the new architecture, it does not modify bugzilla to make any use of it. It is backwards compatible, so it is possible to check in just this patch and deal with modifying bugzilla to use the compatibility layer in another bug, it will not break anything. I split it like this to reduce the size, as the complete patch is nearing 200KB :-(.

This is the second part, modifying bugzilla to make use of the new DB compatibility layer.

Tomas, your patch removes the "abort" part of ThrowUserError() and always unlock tables when this function is called. Is that intentional? I have already opened a bug about this point, bug 277782 (I just added you to the CC list), and I'm waiting for a confirmation before fixing it. If you have a good reason to do this, I would be happy to see it in bug 277782. Also, why some of your requests use Bugzilla->dbh and some others still use SendSQL()? Shouldn't we be consistent?

(In reply to comment #73) > Tomas, your patch removes the "abort" part of ThrowUserError() and always unlock > tables when this function is called. Is that intentional? I have already opened > a bug about this point, bug 277782 (I just added you to the CC list), and I'm > waiting for a confirmation before fixing it. If you have a good reason to do > this, I would be happy to see it in bug 277782. Yes, that's intentional. I have implemented the unlock_tables function in such a way that it knows if any tables are locked or not, so you can safely call it always, but it will unlock only if something is locked. Also, when called from ThrowUserError() (or ThrowCodeError()), it is called with "abort" parameter so if any transaction is in progress, it is rolled back (if transactions are supported). I can remove it from my patch if you want to do it in a separate bug, but you can hardly implement is so flexible and simple without the db-compat layer, so we would have to make it dependent on this bug. It's a few lines however, so I think it's not a big deal to leave it as a part of this patch. What do you think? > > Also, why some of your requests use Bugzilla->dbh and some others still use > SendSQL()? Shouldn't we be consistent? > Yes, we should. But removing SendSQL from the code is a HUGE change and it definitelly should be done in a separate bug. I plan to look at that later, but I don't want to delay this patch because of that. Couple of other bugs are waiting for this and the sooner we get it in, the better...

(In reply to comment #74) > It's a few lines however, so I think it's not a big deal to leave it as a part > of this patch. What do you think? That I cannot find in yours patches where you modify the ThrowUserError() and ThrowUserCode() subroutines. You remove the "abort" parameter when these subroutines are called, but I cannot find where you automatically unlock tables later (if required). Or maybe I misunderstand what you are doing with these two subroutines. > Yes, we should. But removing SendSQL from the code is a HUGE change and it > definitelly should be done in a separate bug. I agree! And I think they are already bugs about that. ;)

(In reply to comment #75) > (In reply to comment #74) > > It's a few lines however, so I think it's not a big deal to leave it as a part > > of this patch. What do you think? > > That I cannot find in yours patches where you modify the ThrowUserError() and > ThrowUserCode() subroutines. ThrowUserError and ThrowCodeError both call _throw_error, ThrowTemplateError doesn't. _throw_error and ThrowTemplateError both call Bugzilla->dbh->sql_unlock_tables('abort'); (i.e. unconditionally). This function is implemented in such a way that is checks for transaction, and if any is in progress, it will unlock tables and rollback the transaction. Otherwise it just returns. Is that what you are looking for? As a side note: Just this morning, I realized this design have a drawback when we start using transactions in the future without actually locking tables - we would call unlock on error without locking first. But that's not an issue now and I will look into addressing it soon. > You remove the "abort" parameter when these > subroutines are called, but I cannot find where you automatically unlock tables > later (if required). Or maybe I misunderstand what you are doing with these two > subroutines. > > > > Yes, we should. But removing SendSQL from the code is a HUGE change and it > > definitelly should be done in a separate bug. > > I agree! And I think they are already bugs about that. ;) > Maybe I should add that AFAIK I haven't added any calls to the obsoleted functions like SendSQL, I just left them as they are. But I was wondering that it would be cleaner to get rid of them completely first, my patch would be simpler. But I don't want to delay it.

+ Yes, removal of SendSQL is bug 278017, and I'm working on it. It *definitely* should not happen in this bug. + As you know, the Throw*Error problem is bug 277782 and should be dealt with there. I'm going to review the patch now, at least a little.

Comment on attachment 172404 [details] [diff] [review] V9, contains DB.pm, MySQL.pm, PostgreSQL.pm changes Yay! I love this. My comments are below. Only a few small changes that I can see (without testing it). ** MySQL.pm ** What is this, by the way? Why do we "use vars" instead of use "our"? >+=item C<sql_last_key()> >+ >+ Description: Outputs SQL syntax for obtaining the last serial number, >+ usually from a previous INSERT. Must be executed directly >+ following the relevant INSERT. >+ Params: ignored >+ Returns: formatted SQL for obtaining last inserted ID (scalar) This documentation only needs to be in Bugzilla::DB, for functions that get "overridden." API documentation should only appear once, and has to be consistent across all modules. The caller doesn't need to know that their parameters are ignored. This comment goes for all of the documentation in this module, pretty much, not just this documentation. Also, I think we should note that note that DBI::last_insert_id (or whatever it's called) is prefered for use over this function, and this function is only for backwards-compatibility, at this point. >+=item C<sql_lock_tables()> Hrm. Functions that don't return SQL shouldn't be named sql_. Maybe bz_lock_tables? This comment goes for all functions that don't return sql. >+ Params: $abort = 'abort' if the operation on locked tables failed >+ (if transactions are supported, the action will be rolled >+ back). No param if the operation succeeded. Don't use strings for enumerated values. If you plan to have more values in the future, you can create them in Bugzilla::Constants. (For example, the way that Bugzilla->login() works, with LOGIN_REQUIRED and LOGIN_OPTIONAL.) If you don't plan to have other reasons besides "abort" in the future, just make this boolean. >+ if (!$self->{private_sql_in_transaction}) { Hrm. In case I forget later in the review, in perl we usually note that things are private by just putting a _ at the beginning of the name. Oh, also, you implemented some _transaction functions in MySQL.pm that are already implemented in DB.pm. I don't think we need to do that, right? >+# Make this module variable so it is shared by all class instances >+my $_cached_supports_transactions = undef; Should be "our", not "my." (Important for mod_perl.) Also, shouldn't this be in Bugzilla::DB, if it's inherited by all subclasses? (Does that work in perl?) >+=item C<sql_start_transaction()> Should these transaction functions note that they are normally called through the "lock" methods? ** PgSQL.pm ** >+sub sql_limit { >+ my ($self, $limit,$offset) = @_; >+ if ($offset) { >+ return " LIMIT $limit OFFSET $offset "; You know, this won't work if $offset = 0. I know that if the OFFSET is 0, then the offset doesn't need to be there, but I'd prefer that the function returns what the programmer *expects* it to return, which is "OFFSET 0". (I can just see some programmer scratching his head at a bug, thinking, "WHY isn't OFFSET showing up in my SQL??" We can just change that to a "defined $offset". >+ Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %read_tables) . >+ ' IN ACCESS SHARE MODE') if keys %read_tables; >+ Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %write_tables) . >+ ' IN EXCLUSIVE MODE') if keys %write_tables; Wow, I still think that should be ROW EXCLUSIVE... ** DB.pm ** It looks like this class is not yet *itself* a real object-oriented class, although its subclasses are. I suppose that's OK for now, if it works. >+# Inherit the class from DBI::db, and Exporter to export deprecated functions >+use Exporter; > use DBI; >+use vars qw(@ISA); >+@ISA = qw(Exporter DBI::db); Wait, why do we have to *inherit* from Exporter? We can just "use" it, right...? >+# derived database modules for instantiation >+use Bugzilla::DB::MySQL; >+use Bugzilla::DB::PostgreSQL; This means that anybody who wants to add a subclass has to modify the parent class, which breaks encapsulation. Instead, within the _connect function, do: require "Bugzilla/DB/" . $dbclass . ".pm"). The $dbclass variable must be the exact name of the .pm file, which is OK. >+=head2 Functions > >-# All this code is backwards compat fu. As such, its a bit ugly. Note the >-# circular dependancies on Bugzilla.pm I'd rather not move the documentation of DB.pm around in this patch. It makes the patch really hard to read, and it will make it look in cvs blame like _all_ of DB.pm was written for this bug. :-) >+ else { >+ # DB module is an "abstract" class which can not be instantiated >+ die "Tried to connect to a database without specifying supported DB driver"; >+ } You should note the type of DB they attempted to connect to, in the error, so admins can figure out what's wrong. Everything else looks good to me.

By the way, here's the way that DBI does it's own inheritance: eval qq{package # hide from PAUSE DBI::_firesafe; # just in case require $driver_class; # load the driver };

Comment on attachment 172405 [details] [diff] [review] V9, makes use of the compatibility layer OK, a few things: (1) These changes should go on another bug. (2) Let's break up the changes into several bugs, so that it's easier to review. So the "changes" bug from point #1 above will be a meta-bug. (I'll still do all the review, if nobody else is interested in doing it, it would just be easier for me to test things if we could break this up.) We can do one bug for "Replace usage of LAST_INSERT_ID with sql_last_insert" and so on, one for each function that should be implemented in Bugzilla. Basically, it's this patch, just broken up across a few different patches to make life easier, and put on several different bugs that all depend on this one being in.

By the way, here's some good ways to implement abstract classes in perl, using the "can" function: http://perlmonks.thepen.com/44265.html

As I am currently at work doing some paid work :-), just quick comments from the top of my head: (In reply to comment #78) > (From update of attachment 172404 [details] [diff] [review] [edit]) > > Yay! I love this. My comments are below. Only a few small changes that I can > see (without testing it). > > ** MySQL.pm ** > > What is this, by the way? Why do we "use vars" instead of use "our"? Do you mean the "use vars qw(@ISA);" line? That's taken from some perl sample about OO and inheritance. I will fix that. > > >+=item C<sql_last_key()> > >+ > >+ Description: Outputs SQL syntax for obtaining the last serial number, > >+ usually from a previous INSERT. Must be executed directly > >+ following the relevant INSERT. > >+ Params: ignored > >+ Returns: formatted SQL for obtaining last inserted ID (scalar) > > This documentation only needs to be in Bugzilla::DB, for functions that get > "overridden." API documentation should only appear once, and has to be > consistent across all modules. The caller doesn't need to know that their > parameters are ignored. This comment goes for all of the documentation in this > module, pretty much, not just this documentation. So you suggest to have the derived modules without documentation at all? Or with just some sort of "see Bugzilla::DB" comment? Yeah, I can do that... > > Also, I think we should note that note that DBI::last_insert_id (or whatever > it's called) is prefered for use over this function, and this function is only > for backwards-compatibility, at this point. Not sure about that. Postgres implements this in quite a different way and I am not sure last_insert_id will ever be supported there. But for other DBs, it probably makes sense to provide default implementation in Bugzilla::DB using last_insert_db. I will fix that. > > >+=item C<sql_lock_tables()> > > Hrm. Functions that don't return SQL shouldn't be named sql_. Maybe > bz_lock_tables? > > This comment goes for all functions that don't return sql. Yeah, I was thinking about that too... I will see what I can do with that. What about do_lock_tables? > > >+ Params: $abort = 'abort' if the operation on locked tables failed > >+ (if transactions are supported, the action will be rolled > >+ back). No param if the operation succeeded. > > Don't use strings for enumerated values. If you plan to have more values in > the future, you can create them in Bugzilla::Constants. (For example, the way > that Bugzilla->login() works, with LOGIN_REQUIRED and LOGIN_OPTIONAL.) If you > don't plan to have other reasons besides "abort" in the future, just make this > boolean. Again, just mimicked what's already in Bugzilla trying to be consistent ('abort' parameter for Throw*Error() in Error.pm). I will change that. > > >+ if (!$self->{private_sql_in_transaction}) { > > Hrm. In case I forget later in the review, in perl we usually note that > things are private by just putting a _ at the beginning of the name. I wanted originally to have this in upper case, as suggested in perl OO documentation. It didn't work because of some magic DBI is playing with namespaces. I took this from DBI doco, from the inheritance sample. I will try if undescore will do the trick. > > Oh, also, you implemented some _transaction functions in MySQL.pm that are > already implemented in DB.pm. I don't think we need to do that, right? Look again, they are different. DB.pm is generic implementation which should work with every well behaving DB (i.e. every DB with transaction support). MySql specific code checks for MySQL version to use/not use transactions. > > >+# Make this module variable so it is shared by all class instances > >+my $_cached_supports_transactions = undef; > > Should be "our", not "my." (Important for mod_perl.) I will fix that, thanks. > > Also, shouldn't this be in Bugzilla::DB, if it's inherited by all subclasses? > (Does that work in perl?) Not needed, as this is used only by MySQL. Postgres supports transactions from really early versions, and other DBs probably too... Or we can make it a variable of BugzillaDB and the transaction support can be conditional always, the derived module will just indicate if it supports it or not. But that beats the purpose for OO design and polymorphism, doesn't it? > > >+=item C<sql_start_transaction()> > > Should these transaction functions note that they are normally called through > the "lock" methods? They are, right now, because Postgres can't unlock tables without ending transaction, but there is no reason they need to be called only from lock/unlock. I would like to see transactions used all over bugzilla, it just makes a lot of sense :-). But it's probably fair to mention in lock/unlock doco that they are implicitly starting/ending transaction, so noone does it explicitly. > > ** PgSQL.pm ** > > >+sub sql_limit { > >+ my ($self, $limit,$offset) = @_; > >+ if ($offset) { > >+ return " LIMIT $limit OFFSET $offset "; > > You know, this won't work if $offset = 0. I know that if the OFFSET is 0, > then the offset doesn't need to be there, but I'd prefer that the function > returns what the programmer *expects* it to return, which is "OFFSET 0". (I can > just see some programmer scratching his head at a bug, thinking, "WHY isn't > OFFSET showing up in my SQL??" > > We can just change that to a "defined $offset". Good one, thanks :-). > > >+ Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %read_tables) . > >+ ' IN ACCESS SHARE MODE') if keys %read_tables; > >+ Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %write_tables) . > >+ ' IN EXCLUSIVE MODE') if keys %write_tables; > > Wow, I still think that should be ROW EXCLUSIVE... This is as close to what MySQL "LOCK TABLES" does as I could find. Do you suggest that current locking is "overkill"? :-) If we are going to revisit locking (and we should do it when we start looking more closely at transactions), we should do it on case by case basis and for both Postgres and MySQL. I would personally prefer just leave it for now and try to get rid of locking in favor of transactions where possible (different bug though). > > ** DB.pm ** > > It looks like this class is not yet *itself* a real object-oriented class, > although its subclasses are. I suppose that's OK for now, if it works. Why do you think so? Because they are two support functions to simplify creation? Or why? > > >+# Inherit the class from DBI::db, and Exporter to export deprecated functions > >+use Exporter; > > use DBI; > >+use vars qw(@ISA); > >+@ISA = qw(Exporter DBI::db); > > Wait, why do we have to *inherit* from Exporter? We can just "use" it, > right...? As stated in the comment, it's neccessary for the deprecated functions (another reason to get rid of them soon). I had there "use" before and as soon as I "use"d DBI, it could not find any of the deprecated functions anymore. It took me good three hours to find this workaround. Don't ask me what magic is DBI playing with namespaces and export tables, but this is the only way I found working... But as it is just to support the deprecated interface, and it will go with it, it probably doesn't matter too much anyway. > > >+# derived database modules for instantiation > >+use Bugzilla::DB::MySQL; > >+use Bugzilla::DB::PostgreSQL; > > This means that anybody who wants to add a subclass has to modify the parent > class, which breaks encapsulation. > > Instead, within the _connect function, do: require "Bugzilla/DB/" . $dbclass > . ".pm"). The $dbclass variable must be the exact name of the .pm file, which > is OK. You need to modify it anyway to do the "if $dbdriver = "MySQL" { $i = MySQL->new(); } call for every supported db. But maybe if we include it just as you suggest, we can call new() without checking the $dbdriver value? Nice, I will look into this. Disadvantage is that you can't probably report nice and meaningful error, all you will get is "Module blahblah.pm not found" sort of generic perl error. > > >+=head2 Functions > > > >-# All this code is backwards compat fu. As such, its a bit ugly. Note the > >-# circular dependancies on Bugzilla.pm > > I'd rather not move the documentation of DB.pm around in this patch. It makes > the patch really hard to read, and it will make it look in cvs blame like _all_ > of DB.pm was written for this bug. :-) Hmmm, I forgot about cvs blame :-(. I tried to clean it up a bit, and I just don't like having deprecated functions at the top of the file. If someone comes looking for functions to use, the first they will see is deprecated cruft. But cvs blame is a strong reason. I will change that back... Another reason to remove SendSQL & Co soon :-). > > >+ else { > >+ # DB module is an "abstract" class which can not be instantiated > >+ die "Tried to connect to a database without specifying supported DB driver"; > >+ } > > You should note the type of DB they attempted to connect to, in the error, so > admins can figure out what's wrong. If we implement the loading mechanism as you suggest above, this will have to go anyway. But yes, you are right. > > Everything else looks good to me. > Excellent, thanks a lot :-). I will try to do the changes ASAP and we can go for second round :-).

> Not sure about that. Postgres implements this in quite a different way and I am > not sure last_insert_id will ever be supported there. You mean that DBD::Pg doesn't support last_insert_id? > Yeah, I was thinking about that too... I will see what I can do with that. What > about do_lock_tables? Hrm, I don't think we should use do_, because you never know if that will conflict with some DBI function in the future. Although I like it, other than that, because it's short. > Look again, they are different. DB.pm is generic implementation which should > work with every well behaving DB (i.e. every DB with transaction support). MySql > specific code checks for MySQL version to use/not use transactions. Oh, I didn't realize that _sql_supports_transactions was only implemented in MySQL.pm. I think that perhaps instead subclasses should just have to implement _sql_supports_transactions, and DB.pm can implement the transaction function. > > Also, shouldn't this be in Bugzilla::DB, if it's inherited by all subclasses? > > (Does that work in perl?) > > Not needed, as this is used only by MySQL. Postgres supports transactions from > really early versions, and other DBs probably too... > > Or we can make it a variable of BugzillaDB and the transaction support can be > conditional always, the derived module will just indicate if it supports it or > not. But that beats the purpose for OO design and polymorphism, doesn't it? Hrm. I don't see how it defeats the purpose, really. But I think it's OK the way it is, if you want to keep it that way. > But it's probably fair to mention in lock/unlock doco that they are implicitly > starting/ending transaction, so noone does it explicitly. Yeah, that's definitely a good idea. > > Wow, I still think that should be ROW EXCLUSIVE... > > This is as close to what MySQL "LOCK TABLES" does as I could find. Do you > suggest that current locking is "overkill"? :-) Hahaha. Well, I'm suggesting that MySQL's locking doesn't allow us to be as fine-grained as we'd like. :-) > If we are going to revisit locking (and we should do it when we start looking > more closely at transactions), we should do it on case by case basis and for > both Postgres and MySQL. I would personally prefer just leave it for now and try > to get rid of locking in favor of transactions where possible (different bug > though). OK. I agree with that. I think that eventually we could also change the way that bz_lock_tables (or whatever it will be called) works so that it takes a hash and the "minimum" required lock type. That's not necessary now, though, I think. > > It looks like this class is not yet *itself* a real object-oriented class, > > although its subclasses are. I suppose that's OK for now, if it works. > > Why do you think so? Because they are two support functions to simplify > creation? Or why? I think it's OK because it *works*. :-) If you want to re-work it to be a real OO class with "new" and so forth, feel free. But not if it will make this patch take a lot longer. :-) > > Wait, why do we have to *inherit* from Exporter? We can just "use" it, > > right...? > > As stated in the comment, it's neccessary for the deprecated functions (another > reason to get rid of them soon). I had there "use" before and as soon as I > "use"d DBI, it could not find any of the deprecated functions anymore. It took > me good three hours to find this workaround. Don't ask me what magic is DBI > playing with namespaces and export tables, but this is the only way I found > working... Weird. Is that also why you removed the %Bugzilla::DB::EXPORT_TAGS code? (I'm unclear on why you did that, too.) > You need to modify it anyway to do the "if $dbdriver = "MySQL" { $i = > MySQL->new(); } call for every supported db. > But maybe if we include it just as you suggest, we can call new() without > checking the $dbdriver value? Nice, I will look into this. Yeah, look at how DBI::install_driver works. > Disadvantage is that you can't probably report nice and meaningful error, all > you will get is "Module blahblah.pm not found" sort of generic perl error. Hrm... True. I think DBI::install_driver might also have some clever code that deals with that. > Excellent, thanks a lot :-). I will try to do the changes ASAP and we can go > for second round :-). Great! I look forward to it. :-)

(In reply to comment #83) > > Not sure about that. Postgres implements this in quite a different way and I am > > not sure last_insert_id will ever be supported there. > > You mean that DBD::Pg doesn't support last_insert_id? Maybe it does in some really recent versions, but not sure about that. There was a long discussion about it on dbd::pg mailing list, but I don't know the result. But it's not only that, MySQL supports it in only very recent versions too (see e.g. http://lists.mysql.com/perl/3227). My current solution is to have last_insert_id in the base class, and both MySQL and Pg modules will override with DB specific queries for the time being. Comments? > > > Yeah, I was thinking about that too... I will see what I can do with that. What > > about do_lock_tables? > > Hrm, I don't think we should use do_, because you never know if that will > conflict with some DBI function in the future. Although I like it, other than > that, because it's short. Ok, If I don't find anything better, bz_ will do. > > > > Or we can make it a variable of BugzillaDB and the transaction support can be > > conditional always, the derived module will just indicate if it supports it or > > not. But that beats the purpose for OO design and polymorphism, doesn't it? > > Hrm. I don't see how it defeats the purpose, really. But I think it's OK the > way it is, if you want to keep it that way. Because your base class implementation have basically two possible implementations, and choosing between them based on some flag. That exactly what polymorphism was meant to solve. > > If we are going to revisit locking (and we should do it when we start looking > > more closely at transactions), we should do it on case by case basis and for > > both Postgres and MySQL. I would personally prefer just leave it for now and try > > to get rid of locking in favor of transactions where possible (different bug > > though). > > OK. I agree with that. I think that eventually we could also change the way > that bz_lock_tables (or whatever it will be called) works so that it takes a > hash and the "minimum" required lock type. That's not necessary now, though, I > think. Agree. Ok, I will change it to row locking for pg and leave it with that for now, once we have all the important stuff in, we can get back to it and fine-tune the locking and transactions. > > > > It looks like this class is not yet *itself* a real object-oriented class, > > > although its subclasses are. I suppose that's OK for now, if it works. > > > > Why do you think so? Because they are two support functions to simplify > > creation? Or why? > > I think it's OK because it *works*. :-) If you want to re-work it to be a real > OO class with "new" and so forth, feel free. But not if it will make this patch > take a lot longer. :-) Ahhhh, I see. I thought that it doesn't matter if the constructor is called new() or connect() :-). But you are right, the constructor is not really constructor, it's external function... I will look at that. > > > > Wait, why do we have to *inherit* from Exporter? We can just "use" it, > > > right...? > > > > As stated in the comment, it's neccessary for the deprecated functions (another > > reason to get rid of them soon). I had there "use" before and as soon as I > > "use"d DBI, it could not find any of the deprecated functions anymore. It took > > me good three hours to find this workaround. Don't ask me what magic is DBI > > playing with namespaces and export tables, but this is the only way I found > > working... > > Weird. Is that also why you removed the %Bugzilla::DB::EXPORT_TAGS code? (I'm > unclear on why you did that, too.) No, the %Bugzilla::DB::EXPORT_TAGS just got moved down with the rest of the deprecated stuff. I will move all this back in the next version, it's really confusing for both reading the patch and for cvs blame. > > > You need to modify it anyway to do the "if $dbdriver = "MySQL" { $i = > > MySQL->new(); } call for every supported db. > > But maybe if we include it just as you suggest, we can call new() without > > checking the $dbdriver value? Nice, I will look into this. > > Yeah, look at how DBI::install_driver works. I looked, and it's not as much better. You still have a hash with mapping from database name to driver module name which you need to keep up to date. I will see if I can use the same table as DBI is using, thus avoiding any need to modify the code when a new module gets added, or if we'll need similar table in our code. But I don't think this is a big problem, if you'll be implementing support for a new DB, I am pretty sure there will be much more changes in bugzilla neccessary anyway than just a line in a table or adding "use module" clause.

(In reply to comment #84) > My current solution is to have last_insert_id in the base class, and both > MySQL and Pg modules will override with DB specific queries for the time > being. Comments? Sounds perfect. > Because your base class implementation have basically two possible > implementations, and choosing between them based on some flag. That exactly > what polymorphism was meant to solve. Oh, that's true. If we really wanted to have Polymorphism, we would make a Bugzilla:: function caled "initdb" or something that picked the right class, and then we wouldn't even instantiate the base Bugzilla::DB class, at all. > > Yeah, look at how DBI::install_driver works. > > I looked, and it's not as much better. You still have a hash with mapping from > database name to driver module name which you need to keep up to date. > I will see if I can use the same table as DBI is using, thus avoiding any need > to modify the code when a new module gets added, or if we'll need similar > table in our code. OK. I'd like to avoid the table, and have the name of the module be the same as the name of the database. > But I don't think this is a big problem, if you'll be implementing support for > a new DB, I am pretty sure there will be much more changes in bugzilla > neccessary anyway than just a line in a table or adding "use module" clause. Well, hopefully one day that won't be true. :-) If I didn't comment on anything, that means my response is "OK!" :-)

I think I figured out the Exporter problem while working on attachment 172628 [details] [diff] [review]. You have to do: use Exporter qw(import); @Bugzilla::User::EXPORT_OK = qw(insert_new_user); The "qw(import)" is the trick.

New, polished and more tested (on mysql, no so much on postgres) version. This time, I am not enclosing the usage part. This applied as it is should not break Bugzilla on MySQL, it will just not add Postgres compatibility yet :-). Patches for using new functions from this layer will follow once this is finished.

Comment on attachment 172868 [details] [diff] [review] V10 >Index: Bugzilla/DB.pm > [snip] >+=head1 NAME >+ >+Bugzilla::DB - Database access routines, using L<DBI> I'd rather you didn't move the current documentation or code *at all*, and that you put all the new documentation down with the current documentation. Otherwise, it looks like diff still makes you the author of almost all of DB.pm. >+sub db_new { > [snip] > > # connect using our known info to the specified db > # Apache::DBI will cache this when using mod_perl >+ my $self = DBI->connect($dsn, $user, $pass, $attributes); In order for this to be a proper object... isn't that actually $class->connect? Also, why does this function exist? It looks like we should just always be using connect_main, because that actually calls _connect which does what we want it to do. Either that, or it should just return _connect, like connect_main does. >Index: template/en/default/global/code-error.html.tmpl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v >retrieving revision 1.43 >diff -u -r1.43 code-error.html.tmpl >--- template/en/default/global/code-error.html.tmpl 3 Jan 2005 20:54:57 -0000 1.43 >+++ template/en/default/global/code-error.html.tmpl 30 Jan 2005 10:33:38 -0000 >@@ -235,6 +235,18 @@ > I could not figure out what you wanted to do. > [% END %] > >+ [% ELSIF error == "nested_transaction" %] >+ Attempt to start a new transaction without finshing previous one first. Because these start with "attempt", they look like an instruction to the user. You'll want to rephrase them to make sure it's clear that it's a code error. >Index: checksetup.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v >retrieving revision 1.329 >diff -r1.329 checksetup.pl And I can't review this part very easily, because it's not a unified diff. >< # This settings are not yet changeable, because other code depends on >< # the fact that we use MySQL and not, say, PostgreSQL. "This setting is not yet changeable."

Oh, also, last_insert_id doesn't return SQL! The Bugzilla::DB::sql_last_key function and the functions that override it do different things!!

(In reply to comment #88) > (From update of attachment 172868 [details] [diff] [review] [edit]) > >Index: Bugzilla/DB.pm > > [snip] > >+=head1 NAME > >+ > >+Bugzilla::DB - Database access routines, using L<DBI> > > I'd rather you didn't move the current documentation or code *at all*, and > that you put all the new documentation down with the current documentation. I really hate that, as I think having the docs next to implementation is MUCH better than stuffed at the end of the file, but I did it. > Otherwise, it looks like diff still makes you the author of almost all of > DB.pm. It still does. Maybe because I really modified almost all of DB.pm? > > >+sub db_new { > > [snip] > > > > # connect using our known info to the specified db > > # Apache::DBI will cache this when using mod_perl > >+ my $self = DBI->connect($dsn, $user, $pass, $attributes); > > In order for this to be a proper object... isn't that actually > $class->connect? No. We are deriving from DBI::db, this is just DBI. Different class, not super class. > > Also, why does this function exist? It looks like we should just always be > using connect_main, because that actually calls _connect which does what we > want it to do. Either that, or it should just return _connect, like > connect_main does. That would make a cycle in the calls :-). connect_main and connect_shadow both call _connect, just with different parameters (thats something which was already there and I think it's good). _connect creates a new class by calling its constructor, which in turn calls constructor of the base class - DB::db_new. As described in the comment, its not called just new, because then the code checking if abstract methods are implemented would pick base class implementation and we would not detect missing constructor (and derived class needs to implement their own constructor, as it needs to construct DSN, which is DB specific). > > >Index: template/en/default/global/code-error.html.tmpl > >=================================================================== > >RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v > >retrieving revision 1.43 > >diff -u -r1.43 code-error.html.tmpl > >--- template/en/default/global/code-error.html.tmpl 3 Jan 2005 20:54:57 -0000 1.43 > >+++ template/en/default/global/code-error.html.tmpl 30 Jan 2005 10:33:38 -0000 > >@@ -235,6 +235,18 @@ > > I could not figure out what you wanted to do. > > [% END %] > > > >+ [% ELSIF error == "nested_transaction" %] > >+ Attempt to start a new transaction without finshing previous one first. > > Because these start with "attempt", they look like an instruction to the > user. You'll want to rephrase them to make sure it's clear that it's a code > error. Changed to 'Attempted'. > > >Index: checksetup.pl > >=================================================================== > >RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v > >retrieving revision 1.329 > >diff -r1.329 checksetup.pl > > And I can't review this part very easily, because it's not a unified diff. Sorry, forgoten diff flag, fixed. > > >< # This settings are not yet changeable, because other code depends on > >< # the fact that we use MySQL and not, say, PostgreSQL. > > "This setting is not yet changeable." > Old code, I am removing it, not adding :-). (In reply to comment #89) > Oh, also, last_insert_id doesn't return SQL! The Bugzilla::DB::sql_last_key > function and the functions that override it do different things!! Good catch, thanks! Fixed.

Ooops, sorry, old version of the file...

Comment on attachment 172928 [details] [diff] [review] V11.1 This all looks good to me. :-) I haven't tested it, but we can do that for the bugs where we actually *use* it. Thanks for explaining db_new. We might want to also add some documentation in db_new that notes that it's a "protected" (in Java terms) function, that's used by the subclass implementations to create a new database. Technically, the "new()" docs should also note that implementors should call db_new once they've set up anything else, to actually create the object. But those are just little things. >'$tabname'. Please check your '$my_db_driver' access.\n"; That should actually be $db_driver in that message, I think, right? The admin never sees $my_db_driver, but he does see $db_driver. And we'll need a docs patch for that, too, at some point.

So, um, let's get this checked-in. :-) Tomas -- please submit a meta-bug for "make Bugzilla use new DB-independent SQL code" and then file bugs that block it, like I discussed in some comment far above. :-) There should be one bug for each sql_ function and for the lock_tables functions. The meta-bug should block bug 98304. Please CC me on all the bugs. :-)

We need some documentation for this bug -- we need to document the new $db_driver localconfig parameter.

(In reply to comment #92) > (From update of attachment 172928 [details] [diff] [review] [edit]) > This all looks good to me. :-) I haven't tested it, but we can do that for the > bugs where we actually *use* it. I did test it on mysql and it does work the same as without the patch - it does not break anything in the code which is currently used (i.e. connection to the database, deprecated functions from DB etc.). With respect to the new functionality, as you say - when we have a patch using it, we can test it as much as neccessary :-). > > Thanks for explaining db_new. We might want to also add some documentation in > db_new that notes that it's a "protected" (in Java terms) function, that's used > by the subclass implementations to create a new database. Hmmm, it does not have to be protected, as long as you are able to provide correct parameters, its normal constructor. But as the class have abstract methods, it just does not make sense to call it directly :-). But yes, we can add some more description to it later. > > Technically, the "new()" docs should also note that implementors should call > db_new once they've set up anything else, to actually create the object. I think that's just implementation detail, which is obvious from current implementation of Mysql and Pg modules. And if you do all the neccessary initialization in you derived class (mainly the DBI connection), you are not even required to call the base class, but that would probably make everything less clear, so I would not recommend it :-). > > But those are just little things. It's nice to get review with just 'little things' at last :-). It was a long journey for this patch, but it was worth it, I really like it now too. > > >'$tabname'. Please check your '$my_db_driver' access.\n"; > > That should actually be $db_driver in that message, I think, right? The admin > never sees $my_db_driver, but he does see $db_driver. And we'll need a docs > patch for that, too, at some point. > Hmmm, don't think so. If I understand checksetup correctly, $my_db_driver is the only variable having meaning there, $db_driver is only in running bugzilla install (comming from Config.pm, resp. localconfig, which checksetup does not use, resp. generate). But probably I just don't understand the question...

Comment on attachment 172928 [details] [diff] [review] V11.1 >-use base qw(Exporter); >+use Exporter qw(import); Actually, as I just discovered, this only works in very modern versions of perl. Just leave the "use base qw(Exporter)" part in the file.

(In reply to comment #95) > > Technically, the "new()" docs should also note that implementors should call > > db_new once they've set up anything else, to actually create the object. > > I think that's just implementation detail, which is obvious from current > implementation of Mysql and Pg modules. And if you do all the neccessary > initialization in you derived class (mainly the DBI connection), you are not > even required to call the base class, but that would probably make everything > less clear, so I would not recommend it :-). Yeah. I think it would be OK to put it in an =undocumented block, maybe. I think that implementation details that explain "helper" functions are OK in API docs. > > That should actually be $db_driver in that message, I think, right? The admin > > never sees $my_db_driver, but he does see $db_driver. And we'll need a docs > > patch for that, too, at some point. > > Hmmm, don't think so. If I understand checksetup correctly, $my_db_driver is the > only variable having meaning there, $db_driver is only in running bugzilla > install (comming from Config.pm, resp. localconfig, which checksetup does not > use, resp. generate). But probably I just don't understand the question... Yeah. It's true that $my_db_driver is the only thing seen by *checksetup*, but $db_driver will be the only thing seen by the actual Bugzilla admin. :-) So $db_driver is what should actually end up in the docs. :-)

Ok, I have updated this to the tip and done the changes requested. I have also fixed a problem in checksetup.pl, which would correctly create $db_driver parameter in localconfig for new installations, but it would not add it to existing, upgrading instals. And I have changed GetFieldDefs() function which someone meanwhile managed to sqeeze into DB.pm :-) to be a method of the DB class. I think that's the right thing to do, but I welcome any comments. (In reply to comment #96) > (From update of attachment 172928 [details] [diff] [review] [edit]) > >-use base qw(Exporter); > >+use Exporter qw(import); > > Actually, as I just discovered, this only works in very modern versions of > perl. > > Just leave the "use base qw(Exporter)" part in the file. > Ok, did it. But that imply deriving the class also from Exporter. Dunno why, but haven't found any workaround and it's even in some examples on Internet. Without that, the deprecated interface is not found by other modules. But as the deprecated stuff should go, it's not a big deal, it's a temporary solution. (In reply to comment #97) > (In reply to comment #95) > > > Technically, the "new()" docs should also note that implementors should call > > > db_new once they've set up anything else, to actually create the object. > > > > I think that's just implementation detail, which is obvious from current > > implementation of Mysql and Pg modules. And if you do all the neccessary > > initialization in you derived class (mainly the DBI connection), you are not > > even required to call the base class, but that would probably make everything > > less clear, so I would not recommend it :-). > > Yeah. I think it would be OK to put it in an =undocumented block, maybe. I > think that implementation details that explain "helper" functions are OK in API > docs. Ok, I have added a note to the new() method documentation in DB.pm. > > > > That should actually be $db_driver in that message, I think, right? The admin > > > never sees $my_db_driver, but he does see $db_driver. And we'll need a docs > > > patch for that, too, at some point. > > > > Hmmm, don't think so. If I understand checksetup correctly, $my_db_driver is the > > only variable having meaning there, $db_driver is only in running bugzilla > > install (comming from Config.pm, resp. localconfig, which checksetup does not > > use, resp. generate). But probably I just don't understand the question... > > Yeah. It's true that $my_db_driver is the only thing seen by *checksetup*, but > $db_driver will be the only thing seen by the actual Bugzilla admin. :-) So > $db_driver is what should actually end up in the docs. :-) I still don't agree :-). This is not a doc, this is a message which get's printed when table creation fails. It prints "... Please check your 'mysql' access.". And you want the 'mysql' to be taken from the $my_db_driver variable to get the correct name of the DB you have configured to be used.

clearing approval since the review status has changed since it was requested

Comment on attachment 173163 [details] [diff] [review] V12 When I try to use this on landfill, I get: can: handle 'Bugzilla::DB::Mysql' is not a hash reference at Bugzilla/DB.pm line 165. Also, you can just: use base qw(DBI::db Exporter); instead of the @ISA stuff.

(In reply to comment #100) > (From update of attachment 173163 [details] [diff] [review] [edit]) > When I try to use this on landfill, I get: > > can: handle 'Bugzilla::DB::Mysql' is not a hash reference at Bugzilla/DB.pm > line 165. Weird, it works for me :-). Maybe different version of perl? What version is landfill using? I don't have access to landfill, so I can't test it there... Or maybe it's just Mysql.pm module did not get created properly. I should probably add more error checking at the 'eval ("use $pkg_module");' line. > > Also, you can just: > > use base qw(DBI::db Exporter); > > instead of the @ISA stuff. > Neat, thanks, I still have to learn a lot in perl :-).

(In reply to comment #101) > Weird, it works for me :-). Very weird. I just tested it on a fresh install, and it definitely doesn't work for me. > Maybe different version of perl? What version is landfill using? "This is perl, v5.8.3 built for i386-linux-thread-multi" > I don't have access to landfill, so I can't test it there... > Or maybe it's just Mysql.pm module did not get created properly. Maybe. But I see the Mysql.pm module correctly in the DB, and checksetup runs without a problem.

(In reply to comment #102) > (In reply to comment #101) > > Weird, it works for me :-). > > Very weird. I just tested it on a fresh install, and it definitely doesn't > work for me. > > > Maybe different version of perl? What version is landfill using? > > "This is perl, v5.8.3 built for i386-linux-thread-multi" > > > I don't have access to landfill, so I can't test it there... > > Or maybe it's just Mysql.pm module did not get created properly. > > Maybe. But I see the Mysql.pm module correctly in the DB, and checksetup runs > without a problem. Hey, it just hit me, I forgot to add DB directory and contents to the list of items checksetup.pl adjust permisions for. So it may be that just the owner/permisions to the directory and/or modules in there are not set-up correctly. Can you, please, verify this? I will fix this tonight.

(In reply to comment #103) > Can you, please, verify this? I will fix this tonight. Actually, they look OK to me: drwxr-x--- 2 mkanat apache 4096 Feb 3 17:22 DB -rw-r----- 1 mkanat apache 4368 Feb 3 17:21 Mysql.pm -rw-r----- 1 mkanat apache 4966 Feb 3 17:09 Pg.pm

The interesting thing is that if I remove the foreach loop that does the can, it works, even though this is the next line: my $dbh = $pkg_module->new($user, $pass, $host, $dbname, $port, $sock);

(In reply to comment #105) > The interesting thing is that if I remove the foreach loop that does the can, it > works, even though this is the next line: > > my $dbh = $pkg_module->new($user, $pass, $host, $dbname, $port, $sock); According to this: http://xarch.tu-graz.ac.at/home/rurban/news/perl/msg00000.html, using use() in this case is bad. I don't know if that's the cause of the problem, but you can try replacing 'eval ("use $pkg_module");' with 'require $pkg_module;' if that helps. I can't test it here right now, I will play with it more tonight.

(In reply to comment #106) > According to this: > http://xarch.tu-graz.ac.at/home/rurban/news/perl/msg00000.html, using use() in > this case is bad. I don't know if that's the cause of the problem, but you can > try replacing 'eval ("use $pkg_module");' with 'require $pkg_module;' if that > helps. I can't test it here right now, I will play with it more tonight. Yeah, I've also tried the "require" thing. It didn't help, as I recall. -Max

OK, new version. I have incorporated the "use base" change and also changed the way abstract methods are checked to be closer to the original example. Hope this fixes the problem. It took me a while to figure out why deprecated exports stops working when I override import() and what to do with it :-).

Comment on attachment 173351 [details] [diff] [review] V13 Yes, this one works correctly! :-)

Comment on attachment 173351 [details] [diff] [review] V13 OK, after looking at the last_insert_id stuff, here's what I've discovered: Only the *MOST* recent DBD-mysql supports last_insert_id. And only DBI 1.43 supports it correctly. Those versions are really too new for us to require them for Bugzilla 2.20. (It's the mysql one that's the deal-breaker.) So bz_last_key has to go.

(In reply to comment #110) > (From update of attachment 173351 [details] [diff] [review] [edit]) > OK, after looking at the last_insert_id stuff, here's what I've discovered: > > Only the *MOST* recent DBD-mysql supports last_insert_id. And only DBI 1.43 > supports it correctly. > > Those versions are really too new for us to require them for Bugzilla 2.20. > (It's the mysql one that's the deal-breaker.) > > So bz_last_key has to go. > Please, look at the patch once more, more closely. The DBI function last_insert_id is used in the base class, correct. But it is overriden in both mysql and pg modules with custom query, with a comment that the overload can be removed once we require version recent enough to get this working with native DBI implementation. Isn't that exactly what you are asking for? :-)

Comment on attachment 173351 [details] [diff] [review] V13 Oh! :-) OK, then. :-) One small thing that justdave noticed, that could be fixed on checkin, is: "Have you read the doc of '$my_db_driver'?\n";" Should be something like: "Have you read the $my_db_driver documentation?"

I understand from Max that these changes are relatively low risk for MySQL users and have gone through multiple levels of revision and review, but I'm concerned about their impact, so please be around in the days after checking this in to watch out for and respond to reports of regressions caused by the checkins.

I'm also going to send an email to developers@ about this. Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.341; previous revision: 1.340 done Checking in config.cgi; /cvsroot/mozilla/webtools/bugzilla/config.cgi,v <-- config.cgi new revision: 1.7; previous revision: 1.6 done Checking in query.cgi; /cvsroot/mozilla/webtools/bugzilla/query.cgi,v <-- query.cgi new revision: 1.138; previous revision: 1.137 done Checking in Bugzilla/Config.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v <-- Config.pm new revision: 1.32; previous revision: 1.31 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.18; previous revision: 1.17 done RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v done Checking in Bugzilla/DB/Mysql.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v <-- Mysql.pm initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v done Checking in Bugzilla/DB/Pg.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v <-- Pg.pm initial revision: 1.1 done

Tomas: Turns out that perl 5.6 has no DBI::db class. So the patch breaks perl5.6.0. I don't think we need to back the patch out yet, though. I suspect there's a simple fix that can get this up again. Why is it that we subclass DBI::db and not DBI? Can we just eliminate that part of the "use base" statement and be OK?

OK, nevermind. The problem is something else. It only happens in config.cgi: Uncaught exception from user code: Uncaught exception from user code: Uncaught exception from user code: Uncaught exception from user code: Uncaught exception from user code: Can't locate DBI/db.pm in @INC (@INC contains: . /opt/perl-5.6.0/lib/5.6.0/i686-linux /opt/perl-5.6.0/lib/5.6.0 /opt/perl-5.6.0/lib/site_perl/5.6.0/i686-linux /opt/perl-5.6.0/lib/site_perl/5.6.0 /opt/perl-5.6.0/lib/site_perl) at (eval 2) line 3. eval 'require DBI::db ;' called at /opt/perl-5.6.0/lib/5.6.0/base.pm line 59

OK, see bug 282505 for the fix. I guess we just exposed some strange perl5.6 bug.

*** Bug 211769 has been marked as a duplicate of this bug. ***

Added to the release notes in bug 286274, as a change that customizers should be aware of.

(In reply to comment #94) > We need some documentation for this bug -- we need to document the new > $db_driver localconfig parameter. Isn't it documented in localconfig itself?

(In reply to comment #120) > Isn't it documented in localconfig itself? There might be install instructions that leave it out somewhere. If there aren't, then this doesn't need docs.

(In reply to Max Kanat-Alexander from comment #121) > There might be install instructions that leave it out somewhere. If there > aren't, then this doesn't need docs. There is already documentation about $db_driver in the documentation (§2.2.1), so no action required here.