Closed Bug 310718 Opened 19 years ago Closed 17 years ago

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

Categories

(Bugzilla :: Database, enhancement)

2.21
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: mkanat, Assigned: xiaoou.wu)

References

Details

Attachments

(5 files, 9 obsolete files)

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.)
It would be great to see this in 2.22, meaning that we should start the review process really soon.
Target Milestone: --- → Bugzilla 2.22
(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.
Attached file Bugzilla::DB::Schema::Oracle module (obsolete) (deleted) —
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 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 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?
Attachment #199196 - Flags: review-
(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().
(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.
(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).
Attached file Bugzilla::DB::Schema::Oracle module (deleted) —
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.
Attachment #199196 - Attachment is obsolete: true
(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" :)
(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.
Status: NEW → ASSIGNED
Depends on: 153129
(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
(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"...
The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
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".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Assignee: lance.larsh → xiaoou.wu
Status: ASSIGNED → NEW
Attached file Bugzilla::DB::Schema::Oracle module (obsolete) (deleted) —
Attachment #256983 - Flags: review?(mkanat)
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 <andrew@dunslane.net>, ># Edward J. Sabol <edwardjsabol@iname.com> ># Max Kanat-Alexander <mkanat@bugzilla.org> ># Lance Larsh <lance.larsh@oracle.com> 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?
Attachment #256983 - Flags: review?(mkanat) → review-
(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 <andrew@dunslane.net>, > ># Edward J. Sabol <edwardjsabol@iname.com> > ># Max Kanat-Alexander <mkanat@bugzilla.org> > ># Lance Larsh <lance.larsh@oracle.com> > 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. :)
(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.
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.
(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).
Attached file Bugzilla::DB::Schema::Oracle module (obsolete) (deleted) —
Add Foreign key support
Attachment #259288 - Flags: review?(mkanat)
Attachment #256983 - Attachment is obsolete: true
Attachment #256983 - Flags: review-
Attached patch Bugzilla:DB:Schema module (deleted) — Splinter Review
1. TYPE changing TINYTEXT => varchar(255) MEDIUMTEXT => TEXT MEDIUMTEXT => LONGTEXT 2. Make some index name shorter
Attachment #259290 - Flags: review?(mkanat)
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.
Attached file Bugzilla::DB::Schema::Oracle module (obsolete) (deleted) —
We have to create an Oracle job to update Oracle Text Index while creating index
Attachment #259288 - Attachment is obsolete: true
Attachment #260580 - Flags: review?(mkanat)
Attachment #259288 - Flags: review?(mkanat)
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.
Attachment #259290 - Flags: review?(mkanat) → review-
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...)
Attached file Bugzilla::DB::Schema::Oracle module (obsolete) (deleted) —
Attachment #266337 - Flags: review?(mkanat)
Comment on attachment 260580 [details] Bugzilla::DB::Schema::Oracle module This looks like an older version of the module.
Attachment #260580 - Attachment is obsolete: true
Attachment #260580 - Flags: review?(mkanat)
Attachment #266337 - Flags: review?(mkanat) → review-
Attached file Bugzilla::DB::Schema::Oracle (obsolete) (deleted) —
Add some new subs and move the code that dealing with empty string and serial into schema.
Attachment #277048 - Flags: review?(mkanat)
Attachment #266337 - Attachment is obsolete: true
Attachment #277048 - Attachment is obsolete: true
Attachment #277048 - Flags: review?(mkanat)
Attached file Bugzilla::DB::Schema::Oracle (deleted) —
Sync Oracle Text Index on commit
Attachment #277342 - Flags: review?(mkanat)
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.
Attachment #277342 - Flags: review?(mkanat) → review-
>> 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.
(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 = '';
Status: NEW → ASSIGNED
(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.
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.
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.
Yes, I think we should do it, if we can. You should also check for insertion of the "kong", right?
Attached file Bugzilla::DB::Schema::Oracle (obsolete) (deleted) —
remove methods that related to checksetup.pl
Attachment #287353 - Flags: review?(mkanat)
Attached patch Bugzilla::DB::Schema patch (obsolete) (deleted) — Splinter Review
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.
Attachment #287355 - Flags: review?(mkanat)
Attachment #287355 - Attachment is obsolete: true
Attachment #287355 - Flags: review?(mkanat)
Attachment #287353 - Attachment is obsolete: true
Attachment #287353 - Flags: review?(mkanat)
Attached file Bugzilla::DB::Schema::Oracle (obsolete) (deleted) —
Attachment #287480 - Flags: review?(mkanat)
Attachment #287480 - Attachment is obsolete: true
Attachment #287480 - Flags: review?(mkanat)
Attached file Bugzilla::DB::Schema::Oracle (deleted) —
Attachment #287481 - Flags: review?(mkanat)
Attached patch Bugzilla::DB::Schema patch (deleted) — Splinter Review
Attachment #287482 - Flags: review?(mkanat)
Comment on attachment 287482 [details] [diff] [review] Bugzilla::DB::Schema patch This part is definitely fine.
Attachment #287482 - Flags: review?(mkanat) → review+
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. :-)
Attachment #287481 - Flags: review?(mkanat) → review+
This needs to be held in the approval queue until bug 310717 and bug 153129 are ready.
Flags: approval?
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: approval? → approval+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: