237862 - (bz-dbcompat) Modules for database-compatibilty related functions (DBCompat)

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[David Lawrence [:dkl]]

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 1

[David Lawrence [:dkl]]

Attachment: DBcompat.pm module containing database compatibilty functions

Comment 2

[Dave Miller [:justdave]]

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

Comment 3

[David Lawrence [:dkl]]

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 4

[David Lawrence [:dkl]]

Attachment: Updated DBcompat.pm with better perlpod documentation

Comment 5

[Max Kanat-Alexander]

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.

Comment 6

[Dave Miller [:justdave]]

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

Comment 7

[David Lawrence [:dkl]]

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

Comment 8

[David Lawrence [:dkl]]

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

Comment 9

[Ed Sabol]

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

Comment 10

[David Lawrence [:dkl]]

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

Comment 11

[Ed Sabol]

(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

Comment 12

[David Lawrence [:dkl]]

Attachment: DBcompat.pm (v2) with addition to Bugzilla.pm for dbi runtime settings

Comment 13

[Bradley Baetz (:bbaetz)]

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 14

[Bradley Baetz (:bbaetz)]

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?

Comment 15

[Max Kanat-Alexander]

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

Comment 16

[Bradley Baetz (:bbaetz)]

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.

Comment 17

[Ed Sabol]

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

Comment 18

[David Lawrence [:dkl]]

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

Comment 19

[David Lawrence [:dkl]]

Attachment: DBcompat.pm (v3)

Comment 20

[Bradley Baetz (:bbaetz)]

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

Comment 21

[Steve Lamm]

(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/[e-mail]/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.

Comment 22

[Bradley Baetz (:bbaetz)]

> 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)

Comment 23

[Bradley Baetz (:bbaetz)]

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

Comment 24

[Dave Miller [:justdave]]

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.

Comment 25

[Peter Weighill]

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";
    }
}

Comment 26

[Max Kanat-Alexander]

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

Comment 27

[Ed Sabol]

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

Comment 28

[David Lawrence [:dkl]]

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

Comment 29

[Peter Weighill]

(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;
         }
     }

Comment 30

[Peter Weighill]

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)";
    }

Comment 31

[Dave Miller [:justdave]]

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.

Comment 32

[Max Kanat-Alexander]

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.

Comment 33

[Tomas Kopal]

Attachment: DBcompat.pm (v4)

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 34

[Tomas Kopal]

Attachment: BZ changes to make use of DBcompat.pm

Comment 35

[Max Kanat-Alexander]

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.

Comment 36

[Max Kanat-Alexander]

> Is there any 

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

Comment 37

[Tomas Kopal]

Attachment: DBcompat.pm (v5)

Comment 38

[Tomas Kopal]

Attachment: BZ changes to make use of DBcompat.pm (v2)

Comment 39

[Tomas Kopal]

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

[David Lawrence [:dkl]]

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 41

[David Lawrence [:dkl]]

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 42

[David Lawrence [:dkl]]

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.

Comment 43

[David Lawrence [:dkl]]

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

Comment 44

[David Lawrence [:dkl]]

(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';

Comment 45

[Tomas Kopal]

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

Comment 46

[Tomas Kopal]

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

Comment 47

[Tomas Kopal]

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

Comment 48

[Tomas Kopal]

Attachment: DBcompat.pm (v6)

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

Comment 49

[Tomas Kopal]

Attachment: BZ changes to make use of DBcompat.pm (v3)

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

Comment 50

[Joel Peshkin]

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 ???

Comment 51

[Tomas Kopal]

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

Comment 52

[Tomas Kopal]

Attachment: DBcompat.pm (v7)

Simplified unlocking of tables on error.

Comment 53

[Tomas Kopal]

Attachment: BZ changes to make use of DBcompat.pm (v4)

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

Comment 54

[Tomas Kopal]

Attachment: BZ changes to make use of DBcompat.pm (v4.1)

Oooops, wrong file :-).

Comment 55

[Max Kanat-Alexander]

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

Comment 56

[Max Kanat-Alexander]

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

Comment 57

[David Lawrence [:dkl]]

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

Comment 58

[Max Kanat-Alexander]

(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. :-)

Comment 59

[Tomas Kopal]

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

Comment 60

[Max Kanat-Alexander]

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

Comment 61

[Tomas Kopal]

(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/?

Comment 62

[Max Kanat-Alexander]

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

Comment 63

[Tomas Kopal]

> 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 64

[Max Kanat-Alexander]

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.

Comment 65

[Max Kanat-Alexander]

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.

Comment 66

[Tomas Kopal]

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

Comment 67

[Max Kanat-Alexander]

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.

Comment 68

[Tomas Kopal]

Attachment: V8, preview of new OO architecture

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? :-)

Comment 69

[Max Kanat-Alexander]

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.

Comment 70

[Tomas Kopal]

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

Comment 71

[Tomas Kopal]

Attachment: V9, contains DB.pm, MySQL.pm, PostgreSQL.pm changes

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

Comment 72

[Tomas Kopal]

Attachment: V9, makes use of the compatibility layer

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

Comment 73

[Frédéric Buclin]

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?

Comment 74

[Tomas Kopal]

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

Comment 75

[Frédéric Buclin]

(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. ;)

Comment 76

[Tomas Kopal]

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

Comment 77

[Max Kanat-Alexander]

+ 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 78

[Max Kanat-Alexander]

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.

Comment 79

[Max Kanat-Alexander]

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 80

[Max Kanat-Alexander]

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.

Comment 81

[Max Kanat-Alexander]

By the way, here's some good ways to implement abstract classes in perl, using
the "can" function:

http://perlmonks.thepen.com/44265.html

Comment 82

[Tomas Kopal]

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 :-).

Comment 83

[Max Kanat-Alexander]

> 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. :-)

Comment 84

[Tomas Kopal]

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

Comment 85

[Max Kanat-Alexander]

(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!" :-)

Comment 86

[Max Kanat-Alexander]

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.

Comment 87

[Tomas Kopal]

Attachment: V10

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 88

[Max Kanat-Alexander]

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

Comment 89

[Max Kanat-Alexander]

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!!

Comment 90

[Tomas Kopal]

Attachment: V11

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

Comment 91

[Tomas Kopal]

Attachment: V11.1

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

Comment 92

[Max Kanat-Alexander]

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.

Comment 93

[Max Kanat-Alexander]

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. :-)

Comment 94

[Max Kanat-Alexander]

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

Comment 95

[Tomas Kopal]

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

[Max Kanat-Alexander]

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.

Comment 97

[Max Kanat-Alexander]

(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. :-)

Comment 98

[Tomas Kopal]

Attachment: V12

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.

Comment 99

[Dave Miller [:justdave]]

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

Comment 100

[Max Kanat-Alexander]

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.

Comment 101

[Tomas Kopal]

(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 :-).

Comment 102

[Max Kanat-Alexander]

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

Comment 103

[Tomas Kopal]

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

Comment 104

[Max Kanat-Alexander]

(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

Comment 105

[Max Kanat-Alexander]

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);

Comment 106

[Tomas Kopal]

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

Comment 107

[Max Kanat-Alexander]

(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

Comment 108

[Tomas Kopal]

Attachment: V13

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 109

[Max Kanat-Alexander]

Comment on attachment 173351 [details] [diff] [review]
V13

Yes, this one works correctly! :-)

Comment 110

[Max Kanat-Alexander]

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.

Comment 111

[Tomas Kopal]

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

[Max Kanat-Alexander]

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?"

Comment 113

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

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.

Comment 114

[Max Kanat-Alexander]

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

Comment 115

[Max Kanat-Alexander]

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?

Comment 116

[Max Kanat-Alexander]

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

Comment 117

[Max Kanat-Alexander]

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

Comment 118

[Max Kanat-Alexander]

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

Comment 119

[Max Kanat-Alexander]

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

Comment 120

[Frédéric Buclin]

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

Comment 121

[Max Kanat-Alexander]

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

Comment 122

[Frédéric Buclin]

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