307764 - CREATE TABLE column definitions should put DEFAULT attribute before NOT NULL constraint

Status: RESOLVED FIXED

(Bugzilla :: Database, enhancement)

Description

[Lance Larsh]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041119
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041119

When constructing the column definitions for CREATE TABLE statements,
Bugzilla::DB::Schema currently puts the NOT NULL constraint before the DEFAULT
... attribute.  This fails on Oracle, which requires the column's DEFAULT
atribute to appear before the column constraints.  MySQL accepts either order,
and I think PostgreSQL does too (since the current order apparently works, yet
the PostgreSQL doc lists the CREATE TABLE syntax with DEFAULT coming before the
column constraints).  After a few minutes of research on the web, it appears
that having DEFAULT before the constraints will work for most databases.

Reproducible: Always

Steps to Reproduce:
(NOTE:  Testing obviously requires a database driver for the DB of interest, and
as of this moment the driver for Oracle isn't done yet since this bug is a
blocks it.  The steps below show how to test the code against any database for
which a driver is available.)
1. Run checksetup.pl on a clean installation.
2. Edit localconfig to set the db_* parameters to point to a fresh db with no
schema created yet.
3. Run checksetup.pl again (this time to create the schema).

Actual Results:  
The error message is a little misleading, but here's the error that Oracle returns:
  DBD::Oracle::db do failed: ORA-00907: missing right parenthesis (DBD ERROR:
  error possibly near <*> indicator at char 307 in 'CREATE TABLE attachments (
      attach_id       integer NOT NULL PRIMARY KEY,
      bug_id  integer NOT NULL,
      creation_ts     date NOT NULL,
      description     varchar2(4000) NOT NULL,
      mimetype        varchar2(4000) NOT NULL,
      ispatch integer,
      filename        varchar(100) NOT NULL,
      submitter_id    integer NOT NULL,
      isobsolete      integer NOT NULL <*>DEFAULT 0,
      isprivate       integer NOT NULL DEFAULT 0
  )')


Expected Results:  
All tables should be created without errors.

Comment 1

[Lance Larsh]

Attachment: Patch to swap order of DEFAULT and NOT NULL

This patch works against MySQL and Oracle databases, but I haven't tried
PostgreSQL.

Not sure whether Joel or Max is the more appropriate reviewer for this (is it
Schema, or is it SQL?), but I'll start by guessing Joel since every patch I've
sent to Max got passed to Joel.  :)

Comment 2

[Max Kanat-Alexander]

Comment on attachment 195449 [details] [diff] [review]
Patch to swap order of DEFAULT and NOT NULL

It's me. :-)

Joel can do this too, but I was passing things off to him because he's the
Search.pm guy. (My saying of "Schema" was a typo.)

Does this also happen with ALTER TABLE statements? (Will those also need to be
modified?)

Comment 3

[Max Kanat-Alexander]

Comment on attachment 195449 [details] [diff] [review]
Patch to swap order of DEFAULT and NOT NULL

Oh, one other comment: Please add a comment saying "DEFAULT must go before NOT
NULL, for Oracle," so that nobody ever changes it back.

Comment 4

[Lance Larsh]

Attachment: Patch to swap order of DEFAULT and NOT NULL

Added the requested comment.

Yes, it is also required to put DEFAULT before NOT NULL in an ALTER TABLE
statement that adds a column.  However, this patch makes the change in the
function that is used by both CREATE TABLE and ALTER TABLE, so this fixes both
cases.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 195473 [details] [diff] [review]
Patch to swap order of DEFAULT and NOT NULL

Looks good. :-)

Comment 6

[Frédéric Buclin]

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