310718 - [Oracle] Bugzilla::DB::Schema::Oracle module

Status: RESOLVED FIXED

(Bugzilla :: Database, enhancement)

Description

[Max Kanat-Alexander]

In order for checksetup to complete on Oracle, this module is of course one of
the things we need to write. I'd imagine this should be fairly straightforward
to write for Oracle. (The Schema modules usually are pretty easy.)

Comment 1

[Max Kanat-Alexander]

It would be great to see this in 2.22, meaning that we should start the review
process really soon.

Comment 2

[Lance Larsh]

(In reply to comment #1)
> It would be great to see this in 2.22, meaning that we should start the review
> process really soon.

I've got an initial draft of this I can post, pending the response to what I
wrote in Bug 310717 comment #2.

Comment 3

[Lance Larsh]

Attachment: Bugzilla::DB::Schema::Oracle module

This schema module seems to work for getting through checksetup.pl.  We may
need to twiddle some of the VARCHAR or BLOB types later.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 199196 [details]
Bugzilla::DB::Schema::Oracle module

OK, it looks basically good. We'll need to override some other methods to
handle the SERIAL fields though, yes? That is, I take it that Oracle doesn't
have some auto_increment type, so we'll need to add a trigger for each field of
SERIAL type. (And we'll also need to drop the trigger if a field stops being
SERIAL type -- we may want to restructure how some of the bz_alter_column code
works to make it easier to override parts.)

Comment 5

[Max Kanat-Alexander]

Comment on attachment 199196 [details]
Bugzilla::DB::Schema::Oracle module

>        MEDIUMTEXT =>   'varchar(4000)',
>        TEXT =>         'varchar(4000)',

  Oh and these won't work. At the least, they need to be 65535 characters long,
since that's the maximum length of a comment. There's no unbounded text type in
Oracle, eh?

Comment 6

[Lance Larsh]

(In reply to comment #4)
> I take it that Oracle doesn't have some auto_increment type, so we'll 
> need to add a trigger for each field of SERIAL type.

Correct, there's no auto-increment datatype, so currently I create a sequence 
and a trigger for each SERIAL column.  That code is currently in 
Bugzilla::DB::Oracle::bz_setup_database().

Comment 7

[Lance Larsh]

(In reply to comment #5)
> >        MEDIUMTEXT =>   'varchar(4000)',
> >        TEXT =>         'varchar(4000)',
>   Oh and these won't work. At the least, they need to be 65535 characters
> long, since that's the maximum length of a comment. There's no unbounded text
> type in Oracle, eh?

VARCHAR is limited to 4000 characters on Oracle.  However, there are two larger 
types for text: LONG (<= 2GB) and CLOB (<=4GB).  But there are limitations 
associated them:  A) it's not possible to create indexes on LONG or CLOB 
columns; and B) there can be at most 1 column of type LONG in any given table. 
(The latter restriction would prevent us from using LONG for MEDIUMTEXT, since 
the 'attachments' table has two MEDIUMTEXT columns, though it seems odd that 
the 'mimetype' column of that table needs to store up to 64k characters...)  I 
was hoping to get away with VARCHAR(4000) so we could keep the indexes, but I 
guess I'll just have to drop any that are on MEDIUMTEXT/TEXT columns.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #6)
> Correct, there's no auto-increment datatype, so currently I create a sequence 
> and a trigger for each SERIAL column.  That code is currently in 
> Bugzilla::DB::Oracle::bz_setup_database().

  OK. It'll have to live in a lot of other places, too, like any code that
generates ALTER TABLE statements.

> A) it's not possible to create indexes on LONG or CLOB 
> columns;

  That's actually true in PostgreSQL too. And, in fact, it's really true also in
MySQL. You'll notice that any indexes which exist on a table like that are
something special, like a FULLTEXT index in MySQL. In PostgreSQL, the indexes
pretty much just don't exist.

> (The latter restriction would prevent us from using LONG for MEDIUMTEXT, since 
> the 'attachments' table has two MEDIUMTEXT columns, though it seems odd that 
> the 'mimetype' column of that table needs to store up to 64k characters...)

  Yeah, the mimetype column doesn't need to be 64k characters. One thing that we
*can* do is file a bug to change all the silly mediumtext fields to varchar.
Almost *nothing* really needs to be mediumtext, except things like the Status
Whiteboard and the comments.

> was hoping to get away with VARCHAR(4000) so we could keep the indexes, but I 
> guess I'll just have to drop any that are on MEDIUMTEXT/TEXT columns.

  And yeah, once again, that won't be a problem, since there aren't any valid
indexes on those fields (except on short_desc, but that really needs to become a
varchar anyhow).

Comment 9

[Lance Larsh]

Attachment: Bugzilla::DB::Schema::Oracle module

Ok, I've converted MEDIUMTEXT/TEXT to CLOBs, which allow up to 4GB of text, and
changed the Bugzilla::DB::Oracle module not to create any indexes marked as
TYPE => FULLTEXT.

Comment 10

[Dave Miller [:justdave]]

(In reply to comment #8)
> Yeah, the mimetype column doesn't need to be 64k characters. One thing that we
> *can* do is file a bug to change all the silly mediumtext fields to varchar.
> Almost *nothing* really needs to be mediumtext, except things like the Status
> Whiteboard and the comments.

See bug 153129, an ancient bug by today's standards, that no one has gotten
around to yet.  Sounds like now would be a really good time.  The summary is,
conveniently enough, "mediumtext abuse" :)

Comment 11

[Lance Larsh]

(In reply to comment #10)
> See bug 153129, an ancient bug by today's standards, that no one has gotten
> around to yet.  Sounds like now would be a really good time.  The summary is,
> conveniently enough, "mediumtext abuse" :)

As it turns out, some queries fail with the version of the schema module that
uses CLOBs (Attachment 199365 [details]), namely queries that do things like "SELECT
DISTINCT $some_CLOB_column FROM ...".  This type of query returns error
ORA-00932 on Oracle because it requires a sort on the CLOB column, which is not
allowed.  Those queries work fine on VARCHAR columns, so for now the schema
module that uses VARCHAR(4000) (i.e., Attachment 199196 [details]) works better, though
it's obviously not suitable for anything other than debugging/experimentation
due to the 4000-character limit.  Rather than try to hunt down and change any
queries that won't work for CLOB columns, I think we should fix Bug 153129 first
and then see if anything else needs to be done.

Comment 12

[Mihail Daskalov]

(In reply to comment #7)
> (In reply to comment #5)
> > >        MEDIUMTEXT =>   'varchar(4000)',
> > >        TEXT =>         'varchar(4000)',
> >   Oh and these won't work. At the least, they need to be 65535 characters
> > long, since that's the maximum length of a comment. There's no unbounded text
> > type in Oracle, eh?
> 
> VARCHAR is limited to 4000 characters on Oracle.  However, there are two larger 
> types for text: LONG (<= 2GB) and CLOB (<=4GB).  But there are limitations 
> associated them:  A) it's not possible to create indexes on LONG or CLOB 
> columns; and B) there can be at most 1 column of type LONG in any given table. 
> (The latter restriction would prevent us from using LONG for MEDIUMTEXT, since 
> the 'attachments' table has two MEDIUMTEXT columns, though it seems odd that 
> the 'mimetype' column of that table needs to store up to 64k characters...)  I 
> was hoping to get away with VARCHAR(4000) so we could keep the indexes, but I 
> guess I'll just have to drop any that are on MEDIUMTEXT/TEXT columns.

Hi,
it is possible to create TEXT indexes in Oracle, just it is a little difficult when searching in a such FULLTEXT indexes. Oracle Text needs to be installed and it is available in all Oracle 9i and upper releases.


I good workaround (not great, but usefull in most cases is) to have the columns defined as CLOB, and have indexes on them create as functional indexes.
e.g. 
create some_index_on_a_table on table( substr(the_clob_column,1,4000));

Then in the queries you could use 
select * from table
where some_conditions
order by substr(the_clob_column,1,4000));

If you want to query on a column you could do:
select * from table
where some_conditions
substr(the_clob_column,1,4000)) like 'some%words';

or if you choose to create both FUNCTION INDEX and DOMAIN INDEX

select * from table
where some_conditions
and
contains(the_clob_column,'some%words',1)
order by score(1);

This would be documented behaviour so it is no problem.
Large data could be stored, and in the first case only first 4000 symbols would be searchable, in the second case the entire string would be searchable, but column would have to be dropped from order by clauses.

Regards,
Mihail

Comment 13

[Mihail Daskalov]

(In reply to comment #11)
> (In reply to comment #10)
> > See bug 153129, an ancient bug by today's standards, that no one has gotten
> > around to yet.  Sounds like now would be a really good time.  The summary is,
> > conveniently enough, "mediumtext abuse" :)
> 
> As it turns out, some queries fail with the version of the schema module that
> uses CLOBs (Attachment 199365 [details] [edit]), namely queries that do things like "SELECT
> DISTINCT $some_CLOB_column FROM ...".  This type of query returns error
> ORA-00932 on Oracle because it requires a sort on the CLOB column, which is not
> allowed.  Those queries work fine on VARCHAR columns, so for now the schema
> module that uses VARCHAR(4000) (i.e., Attachment 199196 [details] [edit]) works better, though
> it's obviously not suitable for anything other than debugging/experimentation
> due to the 4000-character limit.  Rather than try to hunt down and change any
> queries that won't work for CLOB columns, I think we should fix Bug 153129 first
> and then see if anything else needs to be done.

Maybe another bug should be created with title e.g. "SELECT DISTINCT abuse"...

Comment 14

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 15

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 16

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle module

Comment 17

[Max Kanat-Alexander]

Comment on attachment 256983 [details]
Bugzilla::DB::Schema::Oracle module 

># The Initial Developer of the Original Code is Netscape Communications

  Remember to also fix the license here.

># Contributor(s): Andrew Dunstan <[e-mail]>,
>#                 Edward J. Sabol <[e-mail]>
>#                 Max Kanat-Alexander <[e-mail]>
>#                 Lance Larsh <[e-mail]>

  And add yourself.

>    $self->{db_specific} = {
>
>        BOOLEAN =>      'integer',

  Is there any way to differntiate this particular type of integer? It's not all that important, but it can be useful in certain situations. (I wish I had done it with the MySQL module.)

>        TINYTEXT =>     'varchar(1000)',
>        MEDIUMTEXT =>   'varchar(4000)',
>        TEXT =>         'varchar(4000)',

  Nooooooo, that definitely won't work. :-) For the correct sizes, see:

  http://www.bugzilla.org/docs/tip/html/api/Bugzilla/DB/Schema.html#ABSTRACT_DATA_TYPES

>sub _get_create_index_ddl {

  This looks good. So, if you have this, why do you need the fancy code in DB::Oracle to deal with fulltext indexes?

Comment 18

[Xiaoou]

(In reply to comment #17)
> (From update of attachment 256983 [details])
> ># The Initial Developer of the Original Code is Netscape Communications
>   Remember to also fix the license here.
> ># Contributor(s): Andrew Dunstan <[e-mail]>,
> >#                 Edward J. Sabol <[e-mail]>
> >#                 Max Kanat-Alexander <[e-mail]>
> >#                 Lance Larsh <[e-mail]>
>   And add yourself.
> >    $self->{db_specific} = {
> >
> >        BOOLEAN =>      'integer',
>   Is there any way to differntiate this particular type of integer? It's not
> all that important, but it can be useful in certain situations. (I wish I had
> done it with the MySQL module.)

I will go and look what you have done :)

> >        TINYTEXT =>     'varchar(1000)',
> >        MEDIUMTEXT =>   'varchar(4000)',
> >        TEXT =>         'varchar(4000)',
>   Nooooooo, that definitely won't work. :-) For the correct sizes, see:
>  
> http://www.bugzilla.org/docs/tip/html/api/Bugzilla/DB/Schema.html#ABSTRACT_DATA_TYPES

Yes, it just a slipshod way to make the current version to work
in some way.

If we use CLOB instead, maybe we have to do much more change
in other files , just as Larse mentioned above.
(because insert BLOB/CLOB must bind first)

> >sub _get_create_index_ddl {
>   This looks good. So, if you have this, why do you need the fancy code in
> DB::Oracle to deal with fulltext indexes?

The function bz_add_index is used in checksetup.pl in bugzilla 2.22.1,
but in current rc version, it is removed, so that  fancy function will
also be removed in DB::Oracle too. :)  

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> Yes, it just a slipshod way to make the current version to work
> in some way.
> 
> If we use CLOB instead, maybe we have to do much more change
> in other files , just as Larse mentioned above.
> (because insert BLOB/CLOB must bind first)

  Okay. For our purposes, a limit of 65535 characters for a MEDIUMTEXT would be OK.

> The function bz_add_index is used in checksetup.pl in bugzilla 2.22.1,
> but in current rc version, it is removed, so that  fancy function will
> also be removed in DB::Oracle too. :)  

  No, it moved to Bugzilla::Install::DB.

Comment 20

[Dave Miller [:justdave]]

fwiw, Sybase had the same issue with having to bind before inserting into a BLOB field.  You had to insert the row in the table first, then go back and assign the data blob to the BLOB column afterwards.  We ended up with Sybase-specific code in the attachment.cgi file for the attachment insert at the time, with a big if/then/else thing to see which database you were using.  Perhaps we need some abstract way to pass the attachment data off to the DB module instead of trying to insert it directly in the Bugzilla code.

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
> fwiw, Sybase had the same issue with having to bind before inserting into a
> BLOB field.  You had to insert the row in the table first, then go back and
> assign the data blob to the BLOB column afterwards.

  Wow, that's pretty odd. We already do some special handling for BLOBs in bind_param, so hopefully that should take care of things for Oracle.

  As far as non-varchar text fields, we could even put in special bindings for those, if we've gotten rid of all of our needless mediumtext fields (like we did for Summary).

Comment 22

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle module

Add Foreign key support

Comment 23

[Xiaoou]

Attachment: Bugzilla:DB:Schema module

1. TYPE changing
TINYTEXT   => varchar(255)
MEDIUMTEXT => TEXT
MEDIUMTEXT => LONGTEXT

2. Make some index name shorter

Comment 24

[Frédéric Buclin]

Xiaoou, please don't override reviews, even if you have a newer patch coming. This way, we know which patch has been reviewed or not.

Comment 25

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle module

We have to create an Oracle job to update Oracle Text Index while creating index

Comment 26

[Max Kanat-Alexander]

Comment on attachment 259290 [details] [diff] [review]
Bugzilla:DB:Schema  module

You also must update Bugzilla::Install::DB.

Or, if you're just changing what all of these things mean, you need to update SCHEMA_VERSION and then modify deserialize_schema.

Comment 27

[Max Kanat-Alexander]

Comment on attachment 260580 [details]
Bugzilla::DB::Schema::Oracle module

>        TEXT =>         'varchar(4000)',

  By the way, will this support UTF-8? Or does it have to be nvarchar?

  Because our oracle patches must support UTF-8 from the very beginning.

>sub _get_create_index_ddl {

  Why do you have to override both bz_add_index_raw and this? Shouldn't you only have to override this?

  Overriding bz_add_index_raw is very strange.

>sub get_fk_ddl {

  Why did you override this? This looks exactly the same as the superclass function.

  Wait, does that comment above meant Oracle doesn't support ON UPDATE???

>sub _get_fk_name {
>    my ($self, $table, $column, $references) = @_;
>    my $to_column = $references->{COLUMN};
>    $table        = substr($table,0,15) if length($table) > 15;

  I'm a little concerned about this. Have you considered doing some hash of the table_column_to_column instead? Like an MD5 hash, or something? (I'm not sure it would be short enough, though...)

Comment 28

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle module

Comment 29

[Frédéric Buclin]

Comment on attachment 260580 [details]
Bugzilla::DB::Schema::Oracle module

This looks like an older version of the module.

Comment 30

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle

Add some new subs and move the code that dealing with empty string and serial
into schema.

Comment 31

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle

Sync Oracle Text Index on commit

Comment 32

[Max Kanat-Alexander]

Comment on attachment 277342 [details]
Bugzilla::DB::Schema::Oracle

>    while (@fields) {
>      my $field_name = shift @fields;
>      my $field_info = shift @fields;

  Indentation is four spaces, not two. I don't know why I didn't notice this before.

>      # Create triggers to deal with empty string. 
>      if ( $field_info->{TYPE} =~ /varchar|text/i 

  Nit: Capitalize TEXT there, just to be consistent.

>sub _get_create_index_ddl {
>    # Extend superclass method to create Oracle Text indexes 
>    # if index type is FULLTEXT from schema.
>    # Returns a "create index" SQL statement.

  Nit: Put this comment above the function. (And it can be two lines, yes, not three?)
>
>    my($self, $table_name, $index_name, $index_fields, $index_type) = @_;

  Nit: Space between "my" and argument list.

>sub get_fk_ddl {
>    my ($self, $table, $column, $references) = @_;
>    return "" if !$references;
>
>    my $update    = $references->{UPDATE} || 'CASCADE';
>    my $delete    = $references->{DELETE};
>    my $to_table  = $references->{TABLE}  || confess "No table in reference";
>    my $to_column = $references->{COLUMN} || confess "No column in reference";

  You shouldn't have to re-implement that...I'm not sure what to do about that, though.

>    my $fk_name   = $self->_get_fk_name($table, $column, $references);
>
>    my $fk_string = "\n     CONSTRAINT $fk_name FOREIGN KEY ($column)\n"
>                    . "     REFERENCES $to_table($to_column)\n";
>   
>    $fk_string    = $fk_string . "     ON DELETE $delete" if $delete; 
>    
>    if ( $update eq 'CASCADE' ){

  Nit: if ($update =~ /CASCADE/i) {

>    my $tr_str = "CREATE OR REPLACE TRIGGER ". $table . "_uc"

  This all needs to be indented.

>=item C<get_add_column_ddl($table, $column, \%definition, $init_value)>

  Um, there's no reason for you to re-implement this method.

>sub get_alter_column_ddl {

  Hrm. Are all of these changes really necessary? Oracle doesn't support any kind of standard or ANSI syntax for this? And it doesn't have any simpler way to do all of this?

>sub _get_alter_type_sql {

>    # On Oracle, you don't need UNIQUE if you're a PK--it creates
>    # two identical indexes otherwise.
>    $type =~ s/unique//i if $new_def->{PRIMARYKEY};

  Why would $type ever contain 'UNIQUE'?

>sub get_rename_column_ddl {
> [snip]
>        # We have to rename the series also, and fix the default of the series.

  What if there's a foreign key on the column? 

  All the code should think about that, everywhere.

>sub _get_notnull_trigger_ddl {

  Wait, why does this even exist? If you try to insert a NULL into a NOTNULL column, then the DB should throw an *error*, not insert a string.

Comment 33

[Xiaoou]

>> sub get_fk_ddl {
>>     u  my ($self, $table, $column, $references) = @_;
>>        return "" if !$references;
>>        my $update    = $references->{UPDATE} || 'CASCADE';
>>       my $delete    = $references->{DELETE};
>>        my $to_table  = $references->{TABLE}  || confess "No table in reference";
>>        my $to_column = $references->{COLUMN} || confess "No column in reference";
>>     
>
>   You shouldn't have to re-implement that...I'm not sure what to do about that,
> though.
>   

Why shouldn't I need to re-implement this?   Oracle has no direct way to deal with
"UPDATE CASCADE" as mysql does so that I need a trigger to update referrence field, and there are also something differences in syntax.

>
>> =item C<get_add_column_ddl($table, $column, \%definition, $init_value)>
>>     
>
>
>
>   Um, there's no reason for you to re-implement this method.
>
>   
Do you mean that I  just  change "ALTER TABLE $table ADD COLUMN $column " to
"ALTER TABLE $table ADD  $column " after $self->SUPER::get_add_column_ddl ?
>
>  
>> sub get_alter_column_ddl {
>>     
>
>
>
>   Hrm. Are all of these changes really necessary? Oracle doesn't support any
> kind of standard or ANSI syntax for this? And it doesn't have any simpler way
> to do all of this?
>
>
>   
I just implement the methods according to Bugzilla::Schema, if these methods aren't
needed at all, I will remove them.

btw, I am sorry to say that there aren't.
>  
>> sub _get_alter_type_sql {
>>        # On Oracle, you don't need UNIQUE if you're a PK--it creates
>>        # two identical indexes otherwise.
>>        $type =~ s/unique//i if $new_def->{PRIMARYKEY};
>>     
>   Why would $type ever contain 'UNIQUE'?
>   

Sorry, I just copied from the Pg version, I will remove it.
>
>  
>> sub get_rename_column_ddl {
>>     [snip]
>>          # We have to rename the series also, and fix the default of the
>>     
> series.
>
>
>   What if there's a foreign key on the column?
>   All the code should think about that, everywhere.
>
>
>   
I thought about that,  but I found the Pg version didn't implement that either.
And that is much more complex, first I have to drop the old reference, rename
the old reference field, then create a new reference. Is that necessary?
>  
>> sub _get_notnull_trigger_ddl {
>>     
>
>   Wait, why does this even exist? If you try to insert a NULL into a NOTNULL
> column, then the DB should throw an *error*, not insert a string.

you know, actually the empty string '' is a NULL in Oracle, but Schema in Bugzilla
now allow empty string '' when the field is defined as NOT NULL.
and this method is called here
if ( $field_info->{TYPE} =~ /varchar|TEXT/i
               && $field_info->{NOTNULL} ) {
            push (@ddl, _get_notnull_trigger_ddl($table, $field_name));
       }

and it will not deal with those $field_info->{NOTNULL} is empty.

Comment 34

[Max Kanat-Alexander]

(In reply to comment #33)
> >   You shouldn't have to re-implement that...I'm not sure what to do about that,
> > though.
> 
> Why shouldn't I need to re-implement this?

  Oh, I was just saying that you shouldn't have to re-write code that's identical from the superclass. I don't exactly know what to do about that though, that's why I said "I'm not sure what to do about that."

  It's more of a nit. That wasn't an r- reason.

> >> =item C<get_add_column_ddl($table, $column, \%definition, $init_value)>
>
> >   Um, there's no reason for you to re-implement this method.
> >   
> Do you mean that I  just  change "ALTER TABLE $table ADD COLUMN $column " to
> "ALTER TABLE $table ADD  $column " after $self->SUPER::get_add_column_ddl ?

  You mean Oracle doesn't like it when you say "ADD COLUMN", and that's the only difference?

  In that case make a constant called ADD_COLUMN that defaults to 'ADD COLUMN' in Bugzilla::DB::Schema, and then you can override it for you.

> >> sub get_alter_column_ddl {
> >
> >   Hrm. Are all of these changes really necessary? Oracle doesn't support any
> > kind of standard or ANSI syntax for this? And it doesn't have any simpler way
> > to do all of this?
> >   
> I just implement the methods according to Bugzilla::Schema, if these methods
> aren't
> needed at all, I will remove them.
> 
> btw, I am sorry to say that there aren't.

  These are only needed for checksetup.pl. You can leave out alter_column stuff for now, if you want, as long as you implement it before 3.2.

  I don't want to re-implement this whole method in Oracle.pm, so we need to come up with a way to only re-implement the parts that we have to actually change.

> >> sub get_rename_column_ddl {
> >   What if there's a foreign key on the column?
> >   All the code should think about that, everywhere.
> >   
> I thought about that,  but I found the Pg version didn't implement that either.

  Oh, you're right. Neither does the MySQL code. So let's not worry about it right now.

> you know, actually the empty string '' is a NULL in Oracle, but Schema in
> Bugzilla
> now allow empty string '' when the field is defined as NOT NULL.
> and this method is called here
> if ( $field_info->{TYPE} =~ /varchar|TEXT/i
>                && $field_info->{NOTNULL} ) {
>             push (@ddl, _get_notnull_trigger_ddl($table, $field_name));
>        }
> 
> and it will not deal with those $field_info->{NOTNULL} is empty.

  So you're telling me that this statement:

  UPDATE foo SET bar = NULL;

  Calls the same triggers as this statement?:

  UPDATE foo SET bar = '';

Comment 35

[Xiaoou]

(In reply to comment #34)
> > you know, actually the empty string '' is a NULL in Oracle, but Schema in
> > Bugzilla
> > now allow empty string '' when the field is defined as NOT NULL.
> > and this method is called here
> > if ( $field_info->{TYPE} =~ /varchar|TEXT/i
> >                && $field_info->{NOTNULL} ) {
> >             push (@ddl, _get_notnull_trigger_ddl($table, $field_name));
> >        }
> > 
> > and it will not deal with those $field_info->{NOTNULL} is empty.
>   So you're telling me that this statement:
>   UPDATE foo SET bar = NULL;
>   Calls the same triggers as this statement?:
>   UPDATE foo SET bar = '';

No, the triggers work with those fields which are varchar|TEXT type and NOTNULL. That is to say that those fields can not be NULL all the time, so
the UPDATE foo SET bar = NULL will not call the same triggers.

Comment 36

[Max Kanat-Alexander]

  Okay. Here's how things need to work:

  If I put a NULL value into a NOT NULL field, Oracle should throw an error.
  If I put '' into a NOT NULL field, that should be fine.

Comment 37

[Xiaoou]

I think it is difficult to determine a '' which is not null , because a '' is one of that null in Oracle. 
And for text, most of the time it comes with '' but not that "null". Do you think it is really necessary to do that?
if we really want to do that, we should then do it before posting to Oracle, I think.

Comment 38

[Max Kanat-Alexander]

  Yes, I think we should do it, if we can. You should also check for insertion of the "kong", right?

Comment 39

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle

remove methods that related to checksetup.pl

Comment 40

[Xiaoou]

Attachment: Bugzilla::DB::Schema patch

Change type TINYTEXT to varchar(255), MEDIUMTEXT to TEXT, and some index name changed because they are too long. I am not sure what is the right name to change, so you may determine it.

Comment 41

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle

Comment 42

[Xiaoou]

Attachment: Bugzilla::DB::Schema::Oracle

Comment 43

[Xiaoou]

Attachment: Bugzilla::DB::Schema patch

Comment 44

[Max Kanat-Alexander]

Comment on attachment 287482 [details] [diff] [review]
Bugzilla::DB::Schema patch

This part is definitely fine.

Comment 45

[Max Kanat-Alexander]

Comment on attachment 287481 [details]
Bugzilla::DB::Schema::Oracle

>use constant ADD_COLUMN      => 'ADD';

  Nit: You don't need those spaces.

>sub _get_notnull_trigger_ddl {
>      my ($table, $column) = @_;
>
>      my $notnull_sql = "CREATE OR REPLACE TRIGGER "
>                        . " ${table}_${column}"
>                        . " BEFORE INSERT OR UPDATE ON ". $table
>                        . " FOR EACH ROW"
>                        . " BEGIN "
>                        . " IF :NEW.". $column ." IS NULL THEN  "
>                        . " SELECT '" . Bugzilla::DB::Oracle::EMPTY_STRING
>                        . "' INTO :NEW.". $column ." FROM DUAL; "
>                        . " END IF; "
>                        . " END ".$table.";";

  Nit: You don't have to do that with a lot of .'s. You can just have a multi-line statement like we do for our other SQL in Bugzilla, right?

  Otherwise, this looks just fine. :-)

Comment 46

[Max Kanat-Alexander]

This needs to be held in the approval queue until bug 310717 and bug 153129 are ready.

Comment 47

[Max Kanat-Alexander]

Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.86; previous revision: 1.85
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.104; previous revision: 1.103
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Oracle.pm,v
done
Checking in Bugzilla/DB/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Oracle.pm,v  <--  Oracle.pm
initial revision: 1.1
done
Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.92; previous revision: 1.91
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Oracle.pm,v
done
Checking in Bugzilla/DB/Schema/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Oracle.pm,v  <--  Oracle.pm
initial revision: 1.1
done