The db_port parameter defaults to 3306, the MySQL default. Instead, it should default to the current driver's default, and be changeable. We should probably just set it to some "default" value and then just not pass it to DBI if it's unset.

OK, I actually have a patch for this.

Tomas, could you look over this?

Comment on attachment 175876 [details] [diff] [review] Change the defaults for db_port and the way the modules handle it Tomas is not a blessed reviewer, so I'll also need somebody else to look at this. It's very brief.

OK, the other version had two missing semicolons. :-) But this version is still somehow broken, so I'm working on it...

OK, I figured it out. You can't have an empty db_port param.

OK, this one works. :-) Checksetup can now actually run CREATE DATABASE.

CREATE DATABASE actually works on both MySQL and PostgreSQL. Anyhow, it took a lot of work to get that little section to work. But it's good, now.

This bumps the min DBI version to 1.38, so it needs a relnote.

Comment on attachment 175889 [details] [diff] [review] Fix up CREATE DATABASE entirely And also a bit from glob, since Tomas is not a blessed reviewer.

Oh, I forgot a tiny little piece of the patch...

OK, the Search.pm part of the patch is also in here.

Comment on attachment 175890 [details] [diff] [review] 2v (CREATE DATABASE fix) >+ # This message is specific to MySQL, so we leave it as-is. > if (( $sql_vers =~ /^4\.0\.(\d+)/ ) && ($1 < 2)) { > die "\nYour MySQL server is incompatible with Bugzilla.\n" . > " Bugzilla does not support versions 4.x.x below 4.0.2.\n" . > " Please visit http://www.mysql.com/ and download a newer version.\n"; > } the mysql specific version checks should also ensure that the database driver is mysql. what happens if we support a different database server that happens to be at version 4.0.1? > # mysql 4.0.1 and lower do not support CAST > # mysql 3.*.* had a case-sensitive INSTR > # (checksetup has a check for unsupported versions) >- my $server_version = Bugzilla::DB->server_version; >+ my $server_version = $dbh->bz_server_version; > if ($server_version =~ /^3\./) { > $term = "INSTR($ff ,$q)"; > } else { same here, limit the version checking code to mysql.

Comment on attachment 175889 [details] [diff] [review] Fix up CREATE DATABASE entirely It looks good, but I admit I haven't tested this. Few comments: >+use constant REQUIRED_VERSION => '7.02.0000'; >+use constant PROGRAM_NAME => 'PostgreSQL'; >+ We should go for at least Pg 7.3. Postgres 7.2 would need more hacking (e.g. renaming columns) and will cause trouble later when we start looking at full text search. 7.3 is pretty old anyway :-). Unfortunatelly I don't have access to 7.3 installation so I can't verify it actually works. 7.4 works ok. I suppose we can require 7.3 and if someone reports problems, bump it up? >Index: checksetup.pl >@@ -672,7 +672,7 @@ > # How to access the SQL database: > # > $db_host = 'localhost'; # where is the database? >-$db_port = 3306; # which port to use >+$db_port = undef; # which port to use, undef for default > $db_name = 'bugs'; # name of the MySQL database > $db_user = 'bugs'; # user to attach to the MySQL database > ]); We should either document this better, or propagate correct value from DB specific module. This is what gets into localconfig, so we need to document that "undef" corresponds to default port, regardles of used database, and can be overriden with number (example in the comment, so it's clear if it's string or number etc.). The user modifying localconfig need to know exactly what is permitted there... Few more comments: - Checksetup is still checking for DBD::MySQL, but not for DBD::Pg. This is a major problem, so r- here. Unfortunatelly, it will require bigger change in checksetup, because we are currently checking installed modules before we generate localconfig. But without localconfig, we don't know what database we are going to use. We need to reorder this... - $db_sock should be documented in localconfig wrt postgres (does it make any sense to set it for postgres?). - I suppose changing comments mentioning MySQL to talk about DB in general is not a high priority? Different bug? Should we bother at all?

Comment on attachment 175890 [details] [diff] [review] 2v (CREATE DATABASE fix) (In reply to comment #13) > (From update of attachment 175889 [details] [diff] [review] [edit]) Bugger, wrong attachment, sorry. But everything I wrote still holds...

(In reply to comment #12) OK, I can use $dbh->isa. :-) (In reply to comment #13) > We should go for at least Pg 7.3. Yep, agreed. > >-$db_port = 3306; # which port to use > >+$db_port = undef; # which port to use, undef for default > > We should either document this better, or propagate correct value from DB > specific module. We can't read the DB at the time that we're creating localconfig, so we can't get the version out of it. > This is what gets into localconfig, so we need to document > that "undef" corresponds to default port, regardles of used database, See the comment that's right there, for that. > and can > be overriden with number (example in the comment, so it's clear if it's string > or number etc.). Yes, I agree that that needs to be clearer. :-) > The user modifying localconfig need to know exactly what is > permitted there... > Few more comments: > - Checksetup is still checking for DBD::MySQL, but not for DBD::Pg. This is a > major problem, so r- here. That's going to be another bug. It's massively complex. Right now the point of this bug is for Pg to be able to run CREATE DATABASE, not to fix all of checksetup. > - $db_sock should be documented in localconfig wrt postgres (does it make any > sense to set it for postgres?). No, I don't think it makes any sense for PostgreSQL. I'll modify the comments. > - I suppose changing comments mentioning MySQL to talk about DB in general is > not a high priority? Different bug? Should we bother at all? Hrm. Different bug.

OK, I've addressed all the comments.

(In reply to comment #15) > > - Checksetup is still checking for DBD::MySQL, but not for DBD::Pg. This is a > > major problem, so r- here. > > That's going to be another bug. It's massively complex. Right now the point of > this bug is for Pg to be able to run CREATE DATABASE, not to fix all of checksetup. > Ok, I agree with that, raise a new bug. I was thinking about how to do it and I think it does not have to be such a big change. My idea is: - remove DBD module from the hash with module versions. - add a method do DB specific module (probably to the DB schema module as it is needed only at setup time) which will add DB specific items to the hash of module versions. - before checking for module versions, check if db_driver has been set, and call the DB specific method to modify the hash if yes. This way, when you run checksetup first time, it will check only for generic, DB independent modules, and then it will create localconfig. When user updates localconfig and runs checksetup second time, we know which db_driver to use, so we'll check all the modules again, but this time including correct DBD driver. And every database can require as many specific modules as it wants :-). How does that sound? :-)

Comment on attachment 175976 [details] [diff] [review] v3 Yep, this looks good, FWIW its r+ from me by inspection.

Comment on attachment 175976 [details] [diff] [review] v3 r=glob

Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.358; previous revision: 1.357 done Checking in Bugzilla/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm new revision: 1.24; previous revision: 1.23 done Checking in Bugzilla/Search.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm new revision: 1.85; previous revision: 1.84 done Checking in Bugzilla/DB/Mysql.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v <-- Mysql.pm new revision: 1.2; previous revision: 1.1 done Checking in Bugzilla/DB/Pg.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v <-- Pg.pm new revision: 1.2; previous revision: 1.1 done

Added to the release notes in bug 286274.