Of course, one of the very first things we need for Oracle support is an Oracle.pm module in the Bugzilla/DB/ directory. I'd imagine this should be fairly straightforward, and I'd also imagine that you may already have one in progress, Lance? Let's use this bug to post the module as a patch and review it.

If at all possible, I'd really love to see this in 2.22, which is freezing in either two weeks or a month (I think we're not sure, yet).

(In reply to comment #0) > Of course, one of the very first things we need for Oracle support is an > Oracle.pm module in the Bugzilla/DB/ directory. I'd imagine this should be > fairly straightforward, and I'd also imagine that you may already have one in > progress, Lance? Yep, I've got one partially written, but I started just by copying Pg.pm and modifying it as I encounter problems. So I know there's still unmodified code from Pg.pm that just won't work on Oracle (i.e., parts that aren't encountered during checksetup.pl). I can post what I've got if it's ok to post something that's known not to be working yet. Also, would the ability to upgrade the schema be required from the day I post the first version? Or would that only become a requirement once we have the first complete, working version? Since we're still working out compatibility issues at this point, I'd hate to have to lug around silly upgrade code to yank out messy stuff I threw in as stopgap measures while we sort out the compatibility issues. If the ability to upgrade a database from version created *today* isn't yet required, then it should be ok.

(In reply to comment #2) > I can post what I've got if it's ok to post something > that's known not to be working yet. Yeah, that'd be fine. It'd probably be good just to look over it. > Also, would the ability to upgrade the schema be required from the day I post > the first version? Or would that only become a requirement once we have the > first complete, working version? Yeah, you don't have to worry about upgrading the database until you want to worry about that. You can just tell people "you have to drop the whole DB if you want to upgrade, until we say it's OK." That's what we did for Pg.

Here's a first cut at the Oracle DB driver module. This is just an initial version that "mostly" works (pending fixes for other bugs that are blocking Bug 189947), but I'll probably need to do plenty of cleanup before it's ready to go. If you play with this version, DON'T EXPECT TO BE ABLE TO UPGRADE THE RESULTING DATABASE! I haven't even looked at any of the upgrade code yet (e.g., the bz_alter_* methods), and it may turn out that the way I've done some things in this version will make upgrading difficult. If so, I may not bother trying change that stuff in an "upgradeable" way.

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

Comment on attachment 200289 [details] Bugzilla::DB::Oracle module (initial draft) >sub new { > [snip] > $dbname ||= $::db_name || 'ORACLE'; You shouldn't be using a global variable here. If $db_name is empty, it's because we haven't created the database yet. > # construct the DSN from the parameters we got > my $dsn = "DBI:Oracle:host=$host;sid=$dbname"; > $dsn .= ";port=$port" if $port; > > my $attrs = { RaiseError => 0, I think that perhaps we should have an override_attrs that we can pass to db_new, instead. That seems like a better way to do this. That is, we should just be able to specify attrs that we specifically want to override, instead of specifying them all. > LongReadLen => 131072, Why this? That may cause some problems, because we don't do multiple reads on attachments... > # all class local variables stored in DBI derived class needs to have > # a prefix 'private_'. See DBI documentation. > $self->{private_bz_tables_locked} = ""; You know, it really seems like this should be in db_new instead of here, also, now that I think about it. >sub sql_interval { > my ($self, $interval) = @_; Note that we changed the syntax of this function, recently. >sub bz_lock_tables { > [snip] > } else { > my %read_tables; > my %write_tables; > foreach my $table (@tables) { > $table =~ /^([\d\w]+)([\s]+AS[\s]+[\d\w]+)?[\s]+(WRITE|READ)$/i; > my $table_name = $1; > if ($3 =~ /READ/i) { > if (!exists $read_tables{$table_name}) { > $read_tables{$table_name} = undef; > } > } > else { > if (!exists $write_tables{$table_name}) { > $write_tables{$table_name} = undef; > } > } > } Seems like this code may end up in multiple DB Drivers. Might want to have a "protected" subroutine in Bugzilla::DB to just do this for us. >##################################################################### ># DBI/DBD:Oracle Empty String Handling >##################################################################### > >sub adjust_string { > my ($string) = @_; This is pretty much a private function, right? So it should probably start with an underscore. >sub adjust_statement { > my ($sql) = @_; > > # We can't just assume any occurrence of "''" in $sql is an empty > # string, since "''" can occur inside a string literal as a way of > # escaping a single "'" in the literal. Therefore we must be trickier... > > # split the statement into parts by single-quotes. The negative value > # at the end to the split operator from dropping trailing empty strings > # (e.g., when $sql ends in "''") > my @parts = split /'/, $sql, -1; > > if( !(@parts % 2) ) { > # Either the string is empty or the quotes are mismatched > #print " Mismatched quotes! Returning input unmodified\n"; > return $sql; > } > > # number of parts > my @result; > $_ = shift @parts; I'd rather you didn't use $_ here, if you can avoid it. > # Oracle recognizes CURRENT_DATE, but not CURRENT_DATE() > s/\bCURRENT_DATE\b\(\)/CURRENT_DATE/io; Actually, that's also the case for Pg, so I think we fixed it in all of Bugzilla. > # Oracle doesn't have LIMIT, so if we find the LIMIT comment, wrap the > # query with "SELECT * FROM (...) WHERE rownum < $limit" > my $limit; > ($limit) = m(/\* LIMIT (\d*) \*/)o; Can't you just put the "my" on that line with the ($limit)? >sub do { > my($self, $sql, $attr, @bind_values) = @_; > my $new_sql = adjust_statement($sql); You'll want to make sure that $sql is actually SQL and not a prepared statement handle, which DBI allows and I believe we sometimes use, yes? > my @adjusted_values; > while( @bind_values ) { > push @adjusted_values, adjust_string(shift @bind_values); > } >sub prepare { > my($self, $sql, $attr) = @_; > my $new_sql = adjust_statement($sql); > return bless $self->SUPER::prepare($new_sql,$attr), 'Bugzilla::DB::Oracle::st'; >##################################################################### ># Custom Database Setup >##################################################################### > >sub bz_setup_database { > [snip] > $self->do("CREATE OR REPLACE FUNCTION to_days( d DATE ) RETURN INTEGER IS BEGIN RETURN TO_CHAR(TO_DATE(d),'J'); END;"); Why not just use sql_to_days? > $self->do("CREATE OR REPLACE FUNCTION from_days ( days INTEGER ) RETURN DATE IS BEGIN RETURN TO_DATE(days,'J'); END;"); I'd imagine we've eliminated FROM_DAYS, since I don't think Pg supports it. > $self->do("CREATE OR REPLACE FUNCTION from_unixtime ( time_t INTEGER ) RETURN DATE IS BEGIN RETURN TO_DATE('1970-01-01 00:00:00','YYYY-MM-DD HH24:MI:SS')+NUMTODSINTERVAL(time_t,'SECOND'); END;"); I think we've eliminated FROM_UNIXTIME entirely, now. > #Create SEQUENCES and TRIGGERS to emulate SERIAL datatypes > foreach my $table (keys %{ $self->_bz_schema->{abstract_schema} }) { abstract_schema should never be accessed or modified from within Bugzilla::DB. Instead, this should happen inside of Bugzilla::DB::Schema::Oracle. ># Even though we drop all indexes on FULLTEXT columns from the abstract ># schema, checksetup.pl explicitly calls bz_add_index() to add a FULLTEXT ># index BUGS.SHORT_DESC, which fails on Oracle. How did we handle it on Pg? Does it just silently succeed on Pg somehow...?

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 on attachment 256981 [details] Bugzilla::DB::Oracle module ># The Initial Developer of the Original Code is Netscape Communications ># Corporation. Portions created by Netscape are ># Copyright (C) 1998 Netscape Communications Corporation. All ># Rights Reserved. Actually, the initial developer is Oracle, and you should adjust this block appropriately. ># Contributor(s): Dave Miller <davem00@aol.com> ># Gayathri Swaminath <gayathrik00@aol.com> ># Jeroen Ruigrok van der Werven <asmodai@wxs.nl> ># Dave Lawrence <dkl@redhat.com> ># Tomas Kopal <Tomas.Kopal@altap.cz> ># Max Kanat-Alexander <mkanat@bugzilla.org> ># Lance Larsh <lance.larsh@oracle.com> Make sure to include yourself! >use constant REQUIRED_VERSION => '10.1.0'; >use constant PROGRAM_NAME => 'Oracle'; >use constant MODULE_NAME => 'Oracle'; >use constant DBD_VERSION => '1.19'; This data goes into Bugzilla::Constants now, on the trunk. Look for the DB_MODULE constant. >sub new { > $dbname ||= $::db_name || 'ORACLE'; There is no global called $::db_name on the trunk, and anyhow, this is not a good idea. Bugzilla really expects to always connect to some database--is there not always a default database in Oracle? > my $attrs = { RaiseError => 0, We should re-write Bugzila::DB::db_new to just accept an $override_attrs, so that you don't have to re-specify all of them. I can do that in a separate bug, if you want, so we don't really have to worry about it here. > LongReadLen => 1310720, Oh, I think that's going to cause problems. Can you explain why it's there? >sub sql_limit { > my ($self, $limit, $offset) = @_; > > if(defined $offset) > { > return "/* LIMIT $limit $offset */"; > } > return "/* LIMIT $limit */"; Why is it in comments? >sub sql_fulltext_search { > my ($self, $column, $text) = @_; > > return "CONTAINS($column,\'$text\')"; You should use $dbh->quote there instead of just putting quotes directly into the function--this could cause a sql injection otherwise. Also, what does CONTAINS return? And can you sort on it? (Look at how "relevance" is built in Bugzilla/Search.pm on the trunk.) >sub sql_interval { > my ($self, $interval, $units) = @_; > > return "INTERVAL \'$interval\' $units"; Once again, use $self->quote, not raw quotes. >sub sql_group_by { > my ($self, $needed_columns, $optional_columns) = @_; > > if($optional_columns) { > return "GROUP BY $needed_columns,$optional_columns"; > } Does Oracle require those optional columns? MySQL allows you to leave columns out of the GROUP BY even if they're listed in the SELECT. PostgreSQL requires all columns to be in GROUP BY. Is Oracle like PostgreSQL, there? >sub adjust_statement { Where do you fix the NULLs, here, like Lance did? Can't you just adjust_string like he did? I preferred that method much more. > # Oracle need no 'AS' > $nonstring =~ s/\bAS\b//ig; This is very clever. :-) >sub do { > my($self, $sql, $attr, @bind_values) = @_; > my $new_sql = adjust_statement($sql); > return $self->SUPER::do($new_sql,$attr,@bind_values); >} > >sub selectrow_array { > my($self, $stmt, $attr, @bind_values) = @_; > my $new_stmt = (ref $stmt) ? $stmt : adjust_statement($stmt); > if ( wantarray ) { > my @row = $self->SUPER::selectrow_array($new_stmt,$attr,@bind_values); > foreach (@row) { s/^__BZ_EMPTY_STR_KONG__$//o if defined $_; } That needs to be in a constant instead of just being a raw string. Also, just use __BZ_EMPTY_STR__, because that's only 16 characters, and so we could then have a varchar(16). Instead of using a regex, just make it a string comparison--I think that'd be much faster, and I think these are pretty performance-critical areas. These same comments apply everywhere else, as well. >sub selectall_arrayref { > my($self, $stmt, $attr, @bind_values) = @_; > my $new_stmt = (ref $stmt) ? $stmt : adjust_statement($stmt); > my $ref = $self->SUPER::selectall_arrayref($new_stmt,$attr,@bind_values); > > if (defined $ref) { > foreach my $row (@$ref) { > if (ref($row) eq 'ARRAY') { > foreach (@$row) { s/^__BZ_EMPTY_STR_KONG__$//o if defined $_; } > } Also, can we somehow make all of this code more modular? It looks like we're re-writing the same $row-handling code over and over. >sub prepare_cached { Does the architecture of DBD::Oracle really require you to override *every single method*? Isn't there just one that could be modified that's called by the others, that fetches row data? >sub bz_setup_database { > my $self = shift; > $self->SUPER::bz_setup_database(@_); > > # Create a function that returns SYSDATE to emulate MySQL's "NOW()" > $self->do("CREATE OR REPLACE FUNCTION now RETURN DATE IS BEGIN RETURN SYSDATE; END;"); > > # Create procedure createjob Could you comment that more to explain what it does? > # Create NOTNULL triggers to deal with empty string > # Create SEQUENCES and TRIGGERS to emulate SERIAL datatypes Could you comment the block below this? I know nothing about Oracle, really, so it'd be good to know what's going on here. :-) > $notnull_sql = "create or replace trigger ".$table."_notnull_tr" > . " before insert or update on ".$table > . " for each row " Nit: Can you put the SQL comamnds in caps there? > if ( $fields{$field}{TYPE} =~ m/^(INT|SMALL|MEDIUM)SERIAL$/ ) { Why not just match against SERIAL being in the string? > my $seq_name = uc("${table}_${field}_seq"); >##################################################################### ># Custom Schema Information Functions >##################################################################### > ># checksetup.pl explicitly calls bz_add_index() to add a FULLTEXT ># index BUGS.SHORT_DESC, which fails on Oracle. Therefore it's also ># necessary to override bz_add_index to ignore such explicit calls. Just override bz_add_index_raw and then also use that in your code above. That would be much more modular.

(In reply to comment #9) > (From update of attachment 256981 [details]) >> # The Initial Developer of the Original Code is Netscape Communications >> # Corporation. Portions created by Netscape are >> # Copyright (C) 1998 Netscape Communications Corporation. All >> # Rights Reserved. > > Actually, the initial developer is Oracle, and you should adjust this block > appropriately. :-) > >> # Contributor(s): Dave Miller <davem00@aol.com> >> # Gayathri Swaminath <gayathrik00@aol.com> >> # Jeroen Ruigrok van der Werven <asmodai@wxs.nl> >> # Dave Lawrence <dkl@redhat.com> >> # Tomas Kopal <Tomas.Kopal@altap.cz> >> # Max Kanat-Alexander <mkanat@bugzilla.org> >> # Lance Larsh <lance.larsh@oracle.com> > > Make sure to include yourself! > OK. >> use constant REQUIRED_VERSION => '10.1.0'; >> use constant PROGRAM_NAME => 'Oracle'; >> use constant MODULE_NAME => 'Oracle'; >> use constant DBD_VERSION => '1.19'; > > This data goes into Bugzilla::Constants now, on the trunk. Look for the > DB_MODULE constant. > So We have to add the following code in DB_MODULE, 'oracle'=> {db => 'Bugzilla::DB::Oracle', db_version => '10.01.0', dbd => { package => 'DBD-Oracle', module => 'DBD::Oracle', version => '1.19', }, name => 'Oracle'}, >> sub new { >> $dbname ||= $::db_name || 'ORACLE'; > > There is no global called $::db_name on the trunk, and anyhow, this is not a > good idea. Bugzilla really expects to always connect to some database--is there > not always a default database in Oracle? > We do have to specify a $db_name, or Oracle will not connect. Oracle has not default $db_name (or SID) to connect to. FYI, line 229 in Bugzilla/DB.pm will not work in Oracle, because the $db_name is not specified. >> my $attrs = { RaiseError => 0, > > We should re-write Bugzila::DB::db_new to just accept an $override_attrs, so > that you don't have to re-specify all of them. I can do that in a separate bug, > if you want, so we don't really have to worry about it here. > OK. >> LongReadLen => 1310720, > > Oh, I think that's going to cause problems. Can you explain why it's there? > The DBI requires some additional information to allow you to query back LONG/LOB (long/large object) datatypes from a database. The DBI is unable to determine how large a buffer to allocate when fetching columns containing LOB data without LongReadLen attribute, and the SELECT will not work. Without the setting of LongReadLen, the fetchrow_array() call would likely fail when fetching the first row, because the default value for LongReadLen is very small -- typically 80 or less. >> sub sql_limit { >> my ($self, $limit, $offset) = @_; >> >> if(defined $offset) >> { >> return "/* LIMIT $limit $offset */"; >> } >> return "/* LIMIT $limit */"; > > Why is it in comments? > Oracle has not limit function, I make it in a comment just like a placeholder, and in function adjust_statement I replace these placeholders with the Oracle way of row limit. (between line 334 to line 355) >> sub sql_fulltext_search { >> my ($self, $column, $text) = @_; >> >> return "CONTAINS($column,\'$text\')"; > > You should use $dbh->quote there instead of just putting quotes directly into > the function--this could cause a sql injection otherwise. > > Also, what does CONTAINS return? And can you sort on it? (Look at how > "relevance" is built in Bugzilla/Search.pm on the trunk.) > I will use $dbh->quote there. and The CONTAINS return relevance just like MACTCH AGAINST do, though the result is much different. >> sub sql_interval { >> my ($self, $interval, $units) = @_; >> >> return "INTERVAL \'$interval\' $units"; > > Once again, use $self->quote, not raw quotes. > OK. >> sub sql_group_by { >> my ($self, $needed_columns, $optional_columns) = @_; >> >> if($optional_columns) { >> return "GROUP BY $needed_columns,$optional_columns"; >> } > > Does Oracle require those optional columns? MySQL allows you to leave columns > out of the GROUP BY even if they're listed in the SELECT. PostgreSQL requires > all columns to be in GROUP BY. Is Oracle like PostgreSQL, there? > Yes, just like PostgreSQL, Oracle requires all columns to be in GROUP BY. And most importantly, Oracle will not allow alias names in GROUP BY and ORDER BY, So I think some things in Bugzilla/Search.pm have to be changed, right? >> sub adjust_statement { > > Where do you fix the NULLs, here, like Lance did? Can't you just > adjust_string like he did? I preferred that method much more. > Lance'way will lead to some problems when do selecting and inserting. I now use Oracle triggers to deal with NULLs, before inserting, Oracle triggers replace NULLs with __BZ_EMPTY_STR_KONG__ , then insert into tables. > >> # Oracle need no 'AS' >> $nonstring =~ s/\bAS\b//ig; > > This is very clever. :-) > Thanks, :-) >> sub do { >> my($self, $sql, $attr, @bind_values) = @_; >> my $new_sql = adjust_statement($sql); >> return $self->SUPER::do($new_sql,$attr,@bind_values); >> } >> >> sub selectrow_array { >> my($self, $stmt, $attr, @bind_values) = @_; >> my $new_stmt = (ref $stmt) ? $stmt : adjust_statement($stmt); >> if ( wantarray ) { >> my @row = $self->SUPER::selectrow_array($new_stmt,$attr,@bind_values); >> foreach (@row) { s/^__BZ_EMPTY_STR_KONG__$//o if defined $_; } > > That needs to be in a constant instead of just being a raw string. > > Also, just use __BZ_EMPTY_STR__, because that's only 16 characters, and so we > could then have a varchar(16). > > Instead of using a regex, just make it a string comparison--I think that'd be > much faster, and I think these are pretty performance-critical areas. > > These same comments apply everywhere else, as well. I agree. But I think the __BZ_EMPTY_STR_KONG__ is more complex and meaningless than __BZ_EMPTY_STR__, so it will not happen that someone just input a bare __BZ_EMPTY_STR__, and the program just takes it as empty. right? > >> sub selectall_arrayref { >> my($self, $stmt, $attr, @bind_values) = @_; >> my $new_stmt = (ref $stmt) ? $stmt : adjust_statement($stmt); >> my $ref = $self->SUPER::selectall_arrayref($new_stmt,$attr,@bind_values); >> >> if (defined $ref) { >> foreach my $row (@$ref) { >> if (ref($row) eq 'ARRAY') { >> foreach (@$row) { s/^__BZ_EMPTY_STR_KONG__$//o if defined $_; } >> } > > Also, can we somehow make all of this code more modular? It looks like we're > re-writing the same $row-handling code over and over. > Yes. I will try it. >> sub prepare_cached { > > Does the architecture of DBD::Oracle really require you to override *every > single method*? Isn't there just one that could be modified that's called by > the others, that fetches row data? As far as I know, there isn't. Please let me know if you know some of these stuffs, :P > >> sub bz_setup_database { >> my $self = shift; >> $self->SUPER::bz_setup_database(@_); >> >> # Create a function that returns SYSDATE to emulate MySQL's "NOW()" >> $self->do("CREATE OR REPLACE FUNCTION now RETURN DATE IS BEGIN RETURN SYSDATE; END;"); >> >> # Create procedure createjob > > Could you comment that more to explain what it does? Yes. > >> # Create NOTNULL triggers to deal with empty string > >> # Create SEQUENCES and TRIGGERS to emulate SERIAL datatypes > > Could you comment the block below this? I know nothing about Oracle, really, > so it'd be good to know what's going on here. :-) yes, it create triggers to replace NULLs with __BZ_EMPTY_STR_KONG__ . > >> $notnull_sql = "create or replace trigger ".$table."_notnull_tr" >> . " before insert or update on ".$table >> . " for each row " > > Nit: Can you put the SQL comamnds in caps there? Sure. > >> if ( $fields{$field}{TYPE} =~ m/^(INT|SMALL|MEDIUM)SERIAL$/ ) { > > Why not just match against SERIAL being in the string? > I am not sure about whether there are other types of SERIAL, so I just match the using ones. if there is not other SERIAL, then SERIAL only is better, ^-^ >> my $seq_name = uc("${table}_${field}_seq"); > >> ##################################################################### >> # Custom Schema Information Functions >> ##################################################################### >> >> # checksetup.pl explicitly calls bz_add_index() to add a FULLTEXT >> # index BUGS.SHORT_DESC, which fails on Oracle. Therefore it's also >> # necessary to override bz_add_index to ignore such explicit calls. > > Just override bz_add_index_raw and then also use that in your code above. > That would be much more modular. > > I will try it. thanks.

Comment on attachment 257788 [details] Bugzilla::DB::Oracle module From what I can see, this is an old version of the patch. Marking as obsolete to avoid confusion.

User Oracle function or procedure should come before SUPER::bz_setup_database

Comment on attachment 259287 [details] Bugzilla::DB::Oracle module ># The Initial Developer of the Original Code is Oracle ># Corporation. Portions created by Oracle are ># Copyright (C) 2007 Oracle Corporation. All ># Rights Reserved. Nit: Fix the formatting of this paragraph. You can probably fit all of that in three lines. ># ># Contributor(s): Dave Miller <davem00@aol.com> ># Gayathri Swaminath <gayathrik00@aol.com> ># Jeroen Ruigrok van der Werven <asmodai@wxs.nl> ># Dave Lawrence <dkl@redhat.com> ># Tomas Kopal <Tomas.Kopal@altap.cz> ># Max Kanat-Alexander <mkanat@bugzilla.org> These people don't need to be here. You can just put you and Lance. If it's a derived work of Bugzilla::DB::Pg, then you can include myself and Tomas. I think we're the only people in that list who still have active code in Bugzilla::DB::Pg. >use constant EMPTY_STRING => '__BZ_EMPTY_STR_KONG__'; Just use _BZ_EMPTY_STR_ -- that fits in 16 characters, if we ever have a varchar(16). (In fact, I think we might already have one, somewhere.) > $dbname ||= $::db_name || 'ORACLE'; There is NO VARIABLE called $::db_name. It's Bugzilla->localconfig->{db_name}. >sub bz_last_key { So, I take it that the normal DBI last_insert_id doesn't work on Oracle? >sub sql_position { > my ($self, $fragment, $text) = @_; > > return "INSTR($text, $fragment)"; So I take it that Oracle doesn't support the ANSI POSITION() function. >sub sql_group_by { > my ($self, $needed_columns, $optional_columns) = @_; So, I take it that Oracle is like Pg, and it requires that any column specified in the SELECT statement must also be in the GROUP BY statement? >sub bz_unlock_tables { This is identical to bz_unlock_tables in Bugzilla::DB, isn't it? So you don't need to override it. >sub do { > my($self, $sql, $attr, @bind_values) = @_; > my $new_sql = adjust_statement($sql); What if $sql is already a prepared statement handle? This does happen, in Bugzilla, I'm pretty sure. >sub selectrow_array { > my($self, $stmt, $attr, @bind_values) = @_; Nit: We usually put a space after "my" there. (Make sure you get that everywhere else, too.) >sub selectall_arrayref { > [snip] > foreach my $row (@$ref) { > if (ref($row) eq 'ARRAY') { > foreach (@$row) { $_ = $self->empty_fix($_) if defined $_; } > } > elsif (ref($row) eq 'HASH') { > foreach my $key (keys %$row) { > $row->{$key} = $self->empty_fix($row->{$key}) if defined $row->{$key}; > } I think it'd be nice to have a _fix_hash and _fix_array subroutine, here, since we seem to be doing this code a few times. >sub prepare { > my($self, $sql, $attr) = @_; > my $new_sql = adjust_statement($sql); > $new_sql =~ s/\n/ /g; Why do you remove newlines? There could theoretically be valid SQL with newlines in it--a string could have newlines. > $new_sql =~ /(.*)/; > $new_sql = $1; Also, WHOA. Don't do that!! That detaints the entire SQL statement unconditionally!! That's realy bad!! >sub prepare_cached { > my($self, $sql, $attr, $if_active) = @_; > my $new_sql = adjust_statement($sql); > $new_sql =~ s/\n/ /g; > $new_sql =~ /(.*)/; > $new_sql = $1; Same here. >sub quote_identifier { > my($self,$id) = @_; > return $id; There's no identifier quoting in Oracle?? This is used when we're using an identifier that might be a reserved word. This makes it impossible to fix reserved words. > package Bugzilla::DB::Oracle::st; > @Bugzilla::DB::Oracle::st::ISA = ('DBD::Oracle::st'); Do "use base" instead. > use constant EMPTY_STRING => '__BZ_EMPTY_STR_KONG__'; Just use the constant from the above package instead. Or make this constant reference that one. > sub empty_fix { Same here--don't re-write this twice, or just have it reference the one above. It's possible you'll need a Bugzilla::DB::Oracle::Util or something. But I don't think so--just don't re-write the same functions/constants twice. >sub bz_setup_database { > [snip] > # Create a function that returns SYSDATE to emulate MySQL's "NOW()". > # Function NOW() is used widely in Bugzilla SQLs, but Oracle has not > # that function, Nit: "does not have" instead of "has not" > # Create procedure createjob > # DBI can not support create Oracle job, but we can create a createjob procedure first > # to create Oracle job. This comment looks longer than 80 characters. Also, I don't understand it. > # Create NOTNULL triggers to deal with empty string > # Create SEQUENCES and TRIGGERS to emulate SERIAL datatypes > foreach my $table (keys %{ $self->_bz_schema->{abstract_schema} }) { > my %fields = ( @{$self->_bz_schema->{abstract_schema}{$table}{FIELDS}} Don't do this this way. Instead, use the standard Schema functions--get_table_list, get_column_list, get_column_abstract, get_column, etc. You should never reference the internals of DB::Schema from inside of DB. I trust that DB::Schema deletes these triggers when they're no longer needed, also, right?

Comment on attachment 260582 [details] [diff] [review] Bugzilla::DB::Oracle patch This patch is no longer required as the new Oracle.pm version already includes it.

Comment on attachment 266336 [details] Bugzilla::DB::Oracle module >sub sql_fulltext_search { > my ($self, $column, $text) = @_; > > return "CONTAINS($column," . $self->quote($text) . ")"; MySQL detaints the text. Perhaps you should do that as well. >sub sql_group_by { I think you don't even need to implement this--it's already implemented in Bugzilla::DB. >sub _fix_arrayref { > [snip] > return undef unless(defined($row)); You don't need any of those parentheses. (Also, it's better to use "if !" than "unless".) > foreach (@$row) { > $_ = _fix_empty($_) if defined $_; > } Use a variable name, don't use $_. >sub _fix_hashref { > my ($row) = @_; > return undef unless(defined($row)); > foreach (values %$row) { > $_ = _fix_empty($_) if defined $_; > } Same comments here. >sub adjust_statement { > [snip] > if( !(@parts % 2) ) { > # Either the string is empty or the quotes are mismatched > #print " Mismatched quotes! Returning input unmodified\n"; > return $sql; > } That should be a ThrowCodeError, not a print statement. The error should also print out the SQL that it's parsing, in a <code> or <ore> tag. > $_ = shift @parts; Don't use $_. Use a real variable. (Actually, never, ever use $_ unless you can't avoid it.) > # Wrap the query with a "WHERE rownum <= ..." if we found LIMIT > if (defined($limit)) { What if there's a LIMIT statement in a sub-select and also in the full statement? > if (!($new_sql =~ /\bWHERE\b/)) { Just use "!~". > my ($beforewhere, $afterwhere) = split /\bWHERE\b/i,$new_sql; What if the word "where" appears inside of a string? > if (defined($offset)) { > if ($new_sql =~ /(.*\s+)from(\s+.*)/i) { What if the word 'from' appears in a string inside of the SELECT part? In general, I would feel more comfortable if you were using a reliable SQL parser from CPAN, instead of parsing the SQL yourself. >sub do { > my ($self, $sql, $attr, @bind_values) = @_; > my $new_sql; > if ( $attr ) { > $new_sql = adjust_statement($sql); > } else { > $new_sql = $sql; > } What? Why only "if ($attr)"? That doesn't make sense. Also, $sql can be a statement handle. _adjust_statement should just return $sql unmodified "if ref $sql". (That way you also don't have to make a lot of "if ref" checks in each method.) > return $self->SUPER::do($new_sql,$attr,@bind_values); In here and the other functions, you rely on the API of DBI never changing or being extended. Instead, you should do: my $self = shift my $sql = shift my $new_sql = ... unshift @_, $sql $self->SUPER::method(@_) That way, you only pull $sql off the argument list, and you always pass the exact argument list up to the SUPER function. >sub quote_identifier { What's this doing here? This should be in upstream DBD::Oracle, shouldn't it? >{ > package Bugzilla::DB::Oracle::st; > @Bugzilla::DB::Oracle::st::ISA = ('DBD::Oracle::st'); This doesn't need to be in a block. Once you write the package statement, everything is inside the package. > sub fetchrow_arrayref { > my ($self) = @_; > my $ref = $self->SUPER::fetchrow_arrayref; You should always pass @_ to SUPER, even if it's empty. (In case the API changes in the future.) But if you do that, you should do "my $self = shift", not "my ($self) = @_". > return undef unless(defined($ref)); Should be "if !defined $ref" (that's easier to read). > sub fetchrow_hashref { > my ($self) = @_; > my $ref = $self->SUPER::fetchrow_hashref; > > return undef unless(defined($ref)); > > Bugzilla::DB::Oracle::_fix_hashref($ref); > > return $ref; Nit: That doesn't need all those extra newlines. > # The key names in the outer HASH ref are the distinct values > # from the $keyfield column returned by the query, so it's > # concievable that we'll find a key EMPTY_STRING which > # needs to be converted to key '' Could you explain a situation where this would happen? (Just in this bug, it doesn't need to be in the comment, unless you can make it very short.) > if (defined $ref->{Bugzilla::DB::Oracle::EMPTY_STRING}) { Use "exists" instead of "defined" here. >sub bz_setup_database { > [snip] > # Oracle Text is a powerful search technology built into all Oracle > # Database editions. It uses standard SQL to index, search, and analyze > # text and documents stored in the Oracle database, in files, and on the > # web. Oracle Text can perform linguistic analysis on documents, as well > # as search text using a variety of strategies including keyword searching, > # context queries, Boolean operations, pattern matching, mixed thematic > # queries, HTML/XML section searching, and so on. This comment doesn't need to be here. > # This createjob procedure creates a job to synchronize index 'indexname' > # every ten minutes. So fulltext will always be ten minutes behind? Can't we make this a trigger that happens whenever a fulltext field is updated? > # Create NOTNULL triggers to deal with empty string > # Create SEQUENCES and TRIGGERS to emulate SERIAL datatypes Couldn't this go into DB::Schema, and just be returned as additional SQL as part of get_table_ddl? Wouldn't that make more sense? > foreach my $table ($self->_bz_schema->get_table_list()) { Since you have it, you should use _bz_real_schema. When in doubt, use _bz_real_schema. > my %fields = (@{ ${$self->_bz_schema->get_table_abstract($table)} > {FIELDS} }); You should not be doing that from Bugzilla::DB. Instead, you should be using get_table_columns, and then doing get_column for each column. The only places that may access the internals of the Schema object are Bugzilla::DB::Schema and Bugzilla::DB::Schema::Oracle. > foreach my $field (keys %fields) { > if ( $fields{$field}{TYPE} =~ /varchar|text/i ) { That should be /char|text/i, right? > if ( defined($fields{$field}{NOTNULL}) > && $fields{$field}{NOTNULL} eq "1" ) { You shouldn't be using "eq" there, and you should never test for truth by checking "== 1". Just don't put an "eq" or an "==" there at all. > if ( $fields{$field}{TYPE} =~ m/^(INT|SMALL|MEDIUM)SERIAL$/ ) { Why not just "/SERIAL/"? > my $seq_name = uc("${table}_${field}_seq"); Why do you "uc" that? You didn't uc it in bz_last_key. > my $dbh = $self->prepare("SELECT object_name FROM " > . " user_objects where object_name = " > . $self->quote($seq_name)); This SQL, and a lot of the other SQL in this function, is badly formatted. And that's not a $dbh, it's a $sth. Also, you should be using placeholders. You should have something like: my $sth = $self->prepare(qq{SELECT object_name FROM user_objects WHERE object_name = ?}); $sth->execute($seq_name); Please see: http://www.bugzilla.org/docs/developer.html#sql-style > $dbh->execute(); > $dbh->fetchrow_array(); > if ($dbh->rows){ > $self->do("DROP SEQUENCE $seq_name"); > } Why are yo using ->rows? Doesn't that not even work in Oracle? Just do "if $sth->fetchrow_array" or something similar. Or just use selectrow_array and don't use prepare/execute/fetch at all. (I'd prefer the latter.) > $self->do("CREATE SEQUENCE $seq_name > INCREMENT BY 1 > START WITH $lastid > NOMAXVALUE > NOCYCLE > CACHE 10"); Nit: Does that all have to be on separate lines? With your next attachment, also attach your changes to Bugzilla::DB and Bugzilla::Constants that are required for this module to work.

> So fulltext will always be ten minutes behind? Can't we make this a trigger > that happens whenever a fulltext field is updated? Yes, we can. But it may take time to update the index and file the bug, considering this, we create a job to update the index background every 10 minutes. >> # Create NOTNULL triggers to deal with empty string >> # Create SEQUENCES and TRIGGERS to emulate SERIAL datatypes > Couldn't this go into DB::Schema, and just be returned as additional SQL as > part of get_table_ddl? Wouldn't that make more sense? This part has been moved to DB::Schema get_table_ddl, you can find new Bugzilla::DB::Schema::Oracle at bug 310718

changes in Bugzilla::DB and Bugzilla::Constants, also some changes in Bugzilla::DB::Schema.

Comment on attachment 277047 [details] [diff] [review] Diff for Bugzilla::DB and Bugzilla::Constants >diff -ru ./Bugzilla/Install/DB.pm ../../bug31/Bugzilla/Install/DB.pm >@@ -2440,8 +2441,7 @@ > my $open_bugs_query_base_new = >- join("&", map { "bug_status=" . url_quote($_) } >- ('NEW', 'REOPENED', 'ASSIGNED', 'UNCONFIRMED')); >+ join("&", map { "bug_status=" . url_quote($_) } BUG_STATE_OPEN); Don't revert that. BUG_STATE_OPEN must not be used in this module, see bug 391710.

(In reply to comment #19) > Yes, we can. > But it may take time to update the index and file the bug, considering this, we > create a job to update the index background every 10 minutes. It's fine to take some time to update the bug, unless it's some ridiculous amount of time. Will it be over 30 seconds?

(In reply to comment #21) > (From update of attachment 277047 [details] [diff] [review]) > >diff -ru ./Bugzilla/Install/DB.pm ../../bug31/Bugzilla/Install/DB.pm > >@@ -2440,8 +2441,7 @@ > > my $open_bugs_query_base_new = > >- join("&", map { "bug_status=" . url_quote($_) } > >- ('NEW', 'REOPENED', 'ASSIGNED', 'UNCONFIRMED')); > >+ join("&", map { "bug_status=" . url_quote($_) } BUG_STATE_OPEN); > Don't revert that. BUG_STATE_OPEN must not be used in this module, see bug > 391710. it is my mistake, that is not a part of the patch, but because the file I diffed is not the same as the file I edited.

(In reply to comment #22) > > create a job to update the index background every 10 minutes. > It's fine to take some time to update the bug, unless it's some ridiculous > amount of time. Will it be over 30 seconds? I have no idea about how long it will take when the table go huge, but I think it will be a cost of commit performance. Also, sync on commit will cost index fragmentation. By the way, most uses of Oracle Text take the method of synchronizing the index periodly.

(In reply to comment #24) > I have no idea about how long it will take when the table go huge, but I think > it will be a cost of commit performance. Also, sync on commit will cost index > fragmentation. > > By the way, most uses of Oracle Text take the method of synchronizing the index > periodly. Okay. However, we don't *know* right now that there's a performance problem. So until we *see* or have reported a performance problem, let's do it on commit (unless you can prove a performance penalty for adding one comment on an installation with 150,000 comments).

Sync the Oracle Text Index on commit. there is also some update in bug 310718

remove some unused code.

I don't get it. Why renaming MEDIUMTEXT to TEXT as you could define MEDIUMTEXT => 'varchar(4000)' in bug 310718?

(In reply to comment #28) > I don't get it. Why renaming MEDIUMTEXT to TEXT as you could define MEDIUMTEXT > => 'varchar(4000)' in bug 310718? You can refer to bug 153129, MEDIUMTEXT abuse.

Comment on attachment 277341 [details] [diff] [review] Diff for Bugzilla::DB and Bugzilla::Constants You need to file all of those Schema changes as a separate bug. See http://wiki.mozilla.org/Bugzilla:Simple_Patches particularly the last section. I can't successfully review one patch that changes every single text field in Bugzilla.

Comment on attachment 277339 [details] Bugzilla::DB::Oracle >sub new { > [snip] > $dbname ||= Bugzilla->localconfig->{db_name} || 'ORACLE'; This won't work, because Bugzilla->localconfig->{db_name} is always "bugs". It defaults to that. It doesn't default to the empty string. > # all class local variables stored in DBI derived class needs to have > # a prefix 'private_'. See DBI documentation. > $self->{private_bz_tables_locked} = ""; Why doesn't this happen in db_new? >sub sql_to_days { > my ($self, $date) = @_; > > return " TO_CHAR(TO_DATE($date),'J') "; You don't have to put spaces around that. > return " TO_DATE($date,'J') "; Same there. >sub sql_fulltext_search { > my ($self, $column, $text) = @_; > $text = $self->quote($text); > trick_taint($text); Whoa, there should *not* be a trick_taint there. If something is tainted coming into here, that's a serious problem. >sub sql_date_format { > my ($self, $date, $format) = @_; > > $format = "%Y.%m.%d %H:%i:%s" if !$format; Put that into a constant in Bugzilla::DB called DEFAULT_DATE_FORMAT. > # Oracle requires a FROM clause in all SELECT statements, so append > # "FROM dual" to queries without one (e.g., "SELECT NOW()") > my $is_select = ($part =~ m/^\s*SELECT\b/io); > my $has_from = ($part =~ m/\bFROM\b/io) if $is_select; What about SELECT statements that have a string in them? They'll be split into two parts. > # Oracle doesn't have LIMIT, so if we find the LIMIT comment, wrap the > # query with "SELECT * FROM (...) WHERE rownum < $limit" That won't change the column order, will it? > my ($limit,$offset) = ($part =~ m(/\* LIMIT (\d*) (\d*) \*/)o); Nit: Usually we use angle-brackets like m{} instead of m(). > if (defined($limit)) { > if ($new_sql !~ /\bWHERE\b/) { > $new_sql = $new_sql." WHERE 1=1"; > } Fix the indentation there. > my ($beforewhere, $afterwhere) = split /\bWHERE\b/i,$new_sql; Nit: $before_where and $after_where. > if (defined($offset)) { Nit: if (defined $offset) > if ($new_sql =~ /(.*\s+)FROM(\s+.*)/i) { > my $beforefrom = $1; > my $afterfrom =$2; Nit: my ($before_from, $after_from) = ($1, $2); Also, fix the indentation. > $afterwhere = " rownum <=".$limit." AND ".$afterwhere; Nit: You can just write " rownum <= $limit AND $afterwhere"; > $new_sql = $beforewhere." WHERE ".$afterwhere; Nit: Same there. Everything else looks fine. :-)

Comment on attachment 287359 [details] [diff] [review] Bugzilla::Constants patch Yes, this part is fine.

Comment on attachment 287358 [details] [diff] [review] Bugzilla::DB patch Can't you just attach the whole thing as one "cvs diff" instead of a bunch of separate patches? >--- Bugzilla/DB.pm 2007-09-08 08:14:27.000000000 +0800 >+++ ../../bug31/Bugzilla/DB.pm 2007-11-05 09:31:27.000000000 +0800 >@@ -229,8 +229,9 @@ > my $dbh; > my $lc = Bugzilla->localconfig; > my $conn_success = eval { >- $dbh = _connect($lc->{db_driver}, $lc->{db_host}, '', $lc->{db_port}, >- $lc->{db_sock}, $lc->{db_user}, $lc->{db_pass}); >+ $dbh = _connect($lc->{db_driver}, $lc->{db_host}, $lc->{db_name}, >+ $lc->{db_port}, $lc->{db_sock}, $lc->{db_user}, >+ $lc->{db_pass}); Um, no, that will break the other DBs. >@@ -301,6 +302,62 @@ >+sub bz_lock_tables { > [snip] This all looks fine, but shouldn't you also remove the Pg bz_lock_tables, now?

Comment on attachment 287356 [details] Bugzilla::DB::Oracle ># Set the language enviroment >$ENV{'NLS_LANG'} = '.UTF8' if Bugzilla->params->{'utf8'}; Whoa. You can't do things like this outside of a subroutine. First of all, it won't work in mod_perl, and then secondly, any time somebody "use"s your module, even if they're just testing, it'll modify the whole environment of their Perl program! Also, what if we're on Windows? And finally, what if NLS_LANG already ends in .UTF8? >sub new { > my ($class, $user, $pass, $host, $dbname, $port) = @_; > > # construct the DSN from the parameters we got > my $dsn = "DBI:Oracle:host=$host;sid=$dbname"; > $dsn .= ";port=$port" if $port; > my $attrs = { FetchHashKeyName => 'NAME_lc', > LongReadLen => 1048576, > }; See also bug 363153--does DBD::Oracle have a flag that we can use to make it return strings with the utf8 bit set? > $self->do("ALTER SESSION SET NLS_LENGTH_SEMANTICS='CHAR'") > if Bugzilla->params->{'utf8'}; That "if" should only be indented four spaces relative to the line above it. Is that all we have to set in order for Oracle to do the following? * Sort as Unicode. * Accept Unicode as input * Give Unicode as output to DBD::Oracle > my $create_lex = $self->prepare("BEGIN CTX_DDL.CREATE_PREFERENCE > ('BZ_LEX', 'WORLD_LEXER'); END;"); > $create_lex->execute(); Nit: You could just use do() instead.

>> # Set the language enviroment >> $ENV{'NLS_LANG'} = '.UTF8' if Bugzilla->params->{'utf8'}; >> > > Whoa. You can't do things like this outside of a subroutine. First of all, it > won't work in mod_perl, and then secondly, any time somebody "use"s your > module, even if they're just testing, it'll modify the whole environment of > their Perl program! > > Also, what if we're on Windows? > > And finally, what if NLS_LANG already ends in .UTF8? > > All I can do is move that code to sub new{}. Oracle uses the NLS_LANG environment variable to indicate what character set is being used on the client. so this NLS_LANG should be set. Also it should work on Windows too. And finally , $ENV{'NLS_LANG'} = '.UTF8' is an assignment. do you have any suggestion? >> sub new { >> my ($class, $user, $pass, $host, $dbname, $port) = @_; >> >> # construct the DSN from the parameters we got >> my $dsn = "DBI:Oracle:host=$host;sid=$dbname"; >> $dsn .= ";port=$port" if $port; >> my $attrs = { FetchHashKeyName => 'NAME_lc', LongReadLen => 1048576, >> }; >> > > See also bug 363153--does DBD::Oracle have a flag that we can use to make it > return strings with the utf8 bit set? > I am sorry to say no. > Is that all we have to set in order for Oracle to do the following? > > * Sort as Unicode. > * Accept Unicode as input > * Give Unicode as output to DBD::Oracle > > Yes, I think it is all to do for Unicode support if you finish the substr issue. But still, data sent to Oracle should be marked as utf8 first.

Move the NLS_LANG to new

(In reply to comment #38) > All I can do is move that code to sub new{}. Oracle uses the NLS_LANG > environment variable to indicate what character set is being used on the > client. so this NLS_LANG should be set. Okay, I suppose that will be fine for now. Note that it will affect other mod_perl apps running on the same server, when running under mod_perl. I take it that it's not used only on connection, but used all the time? That is, there's no SET SESSION that you can do instead of setting the env var? > > See also bug 363153--does DBD::Oracle have a flag that we can use to make it > > return strings with the utf8 bit set? > > > I am sorry to say no. Seriously?? Are you certain?? There's absolutely no way to have it return strings with the utf8 bit set? I thought I read that there was, in the DBD::Oracle docs.

> Okay, I suppose that will be fine for now. Note that it will affect other > mod_perl apps running on the same server, when running under mod_perl. > I take it that it's not used only on connection, but used all the time? That > is, there's no SET SESSION that you can do instead of setting the env var? This NLS_LANG can not be set by ALTER SESSION. > > > See also bug 363153--does DBD::Oracle have a flag that we can use to make it > > > return strings with the utf8 bit set? > > > > > I am sorry to say no. > Seriously?? Are you certain?? There's absolutely no way to have it return > strings with the utf8 bit set? I thought I read that there was, in the > DBD::Oracle docs. I don't find any in the DBD::Oracle, can you show me?

Here: http://search.cpan.org/dist/DBD-Oracle/Oracle.pm#DBD::Oracle_and_Unicode You need to use AL32UTF8 instead of UTF8.

Comment on attachment 287779 [details] Bugzilla::DB::Oracle > $ENV{'NLS_LANG'} = '.UTF8' if Bugzilla->params->{'utf8'}; Wasn't the whole point of this update to set this to AL32UTF8, here?

(In reply to comment #44) > (From update of attachment 287779 [details]) > > $ENV{'NLS_LANG'} = '.UTF8' if Bugzilla->params->{'utf8'}; > > Wasn't the whole point of this update to set this to AL32UTF8, here? in oracle, 'UTF8' = CESU-8 and 'AL32UTF8' = UTF-8 (including 4 byte UTF-8). so, i think bugzilla should use 'AL32UTF8' here.

Comment on attachment 290629 [details] Bugzilla::DB::Oracle Looks good to me.

Comment on attachment 290628 [details] [diff] [review] bz_locktables At the least, this doesn't hurt anything. These functions will probably be gone soon, anyway.

Holding this in the approval queue until bug 153129 is ready.

Bugzilla->params->{'maxattachmentsize'} is undef when first install.

Comment on attachment 292532 [details] [diff] [review] Bugzilla::DB::Oracle patch Fine.

Okay, this patch combines Bugzilla::DB::Oracle, Bugzilla::DB::Schema::Oracle, and a few fixes that I made while installing it on landfill. This will be checked in. Please note that this is highly experimental. Any use of this other than by me and Xiaoou at this point is strongly discouraged. If you try to use it, it will be a difficult experience, and we will not support you. Anybody who wants to play with this can do so at: http://landfill.bugzilla.org/bugzilla-tip-oracle/ Note that much of the functionality is currently broken, such as entering bugs.

Checked in as part of bug 310717.