checksetup cannot run CREATE DATABASE on PostgreSQL

RESOLVED FIXED in Bugzilla 2.20

Status

()

defect
P1
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
Assignee: Tomas.Kopal → mkanat
Tomas, could you look over this?
Attachment #175876 - Flags: review?(Tomas.Kopal)
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.
Attachment #175876 - Flags: review?(bugreport)
Posted patch v1.1 (obsolete) — Splinter Review
OK, the other version had two missing semicolons. :-)

But this version is still somehow broken, so I'm working on it...
Attachment #175876 - Attachment is obsolete: true
Attachment #175876 - Flags: review?(bugreport)
Attachment #175876 - Flags: review?(Tomas.Kopal)
OK, I figured it out. You can't have an empty db_port param.
Status: NEW → ASSIGNED
Summary: localconfig's "db_port" parameter does not make sense in a cross-db context → checksetup cannot run CREATE DATABASE on PostgreSQL
Target Milestone: --- → Bugzilla 2.20
Posted patch v2 (Working Patch) (obsolete) — Splinter Review
OK, this one works. :-) Checksetup can now actually run CREATE DATABASE.
Attachment #175879 - Attachment is obsolete: true
Attachment #175882 - Flags: review?(Tomas.Kopal)
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.
Attachment #175882 - Attachment is obsolete: true
Attachment #175889 - Flags: review?(Tomas.Kopal)
This bumps the min DBI version to 1.38, so it needs a relnote.
Keywords: relnote
Priority: -- → P1
Whiteboard: [relnote comment 8]
Attachment #175882 - Flags: review?(Tomas.Kopal)
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.
Attachment #175889 - Flags: review?(bugzilla)
Oh, I forgot a tiny little piece of the patch...
Posted patch 2v (CREATE DATABASE fix) (obsolete) — Splinter Review
OK, the Search.pm part of the patch is also in here.
Attachment #175889 - Attachment is obsolete: true
Attachment #175890 - Flags: review?(bugzilla)
Attachment #175889 - Flags: review?(bugzilla)
Attachment #175889 - Flags: review?(Tomas.Kopal)
Attachment #175890 - Flags: review?(Tomas.Kopal)
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.
Attachment #175890 - Flags: review?(bugzilla) → review-
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?
Attachment #175889 - Flags: review-
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...
Attachment #175890 - Flags: review?(Tomas.Kopal) → review-
(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.
Posted patch v3Splinter Review
OK, I've addressed all the comments.
Attachment #175890 - Attachment is obsolete: true
Attachment #175976 - Flags: review?(bugzilla)
(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.
Blocks: 284348
Comment on attachment 175976 [details] [diff] [review]
v3

r=glob
Attachment #175976 - Flags: review?(bugzilla) → review+
Flags: approval?
Blocks: 284529
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Added to the release notes in bug 286274.
Keywords: relnote
Whiteboard: [relnote comment 8]
Blocks: 335743
You need to log in before you can comment on or make changes to this bug.