r60665 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r60664‎ | r60665 | r60666 >
Date:13:35, 5 January 2010
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
fixme for r58356
Modified paths:
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -44,10 +44,10 @@
4545 $array_out[] = $item;
4646 }
4747 }
48 -
 48+
4949 return $array_out;
5050 }
51 -
 51+
5252 function __construct( &$db, $stmt, $unique = false ) {
5353 $this->db =& $db;
5454
——@@ -132,7 +132,7 @@
133133 $this->is_key = ( $this->is_pk || $this->is_unique || $this->is_multiple );
134134 $this->type = $info['data_type'];
135135 }
136 -
 136+
137137 function name() {
138138 return $this->name;
139139 }
——@@ -152,7 +152,7 @@
153153 function nullable() {
154154 return $this->nullable;
155155 }
156 -
 156+
157157 function isKey() {
158158 return $this->is_key;
159159 }
——@@ -182,6 +182,8 @@
183183
184184 var $defaultCharset = 'AL32UTF8';
185185
 186+ var $mFileInfoCache = array();
 187+
186188 function __construct( $server = false, $user = false, $password = false, $dbName = false,
187189 $failFunction = false, $flags = 0, $tablePrefix = 'get from global' )
188190 {
——@@ -251,7 +253,7 @@
252254 }
253255
254256 $this->mOpened = true;
255 -
 257+
256258 # removed putenv calls because they interfere with the system globaly
257259 $this->doQuery( 'ALTER SESSION SET NLS_TIMESTAMP_FORMAT=\'DD-MM-YYYY HH24:MI:SS.FF6\'' );
258260 $this->doQuery( 'ALTER SESSION SET NLS_TIMESTAMP_TZ_FORMAT=\'DD-MM-YYYY HH24:MI:SS.FF6\'' );
——@@ -296,10 +298,10 @@
297299 error_reporting( $olderr );
298300
299301 $sql = preg_replace( '/^EXPLAIN /', 'EXPLAIN PLAN SET STATEMENT_ID = \'' . $explain_id . '\' FOR', $sql, 1, $explain_count );
300 -
301 -
 302+
 303+
302304 $olderr = error_reporting( E_ERROR );
303 -
 305+
304306 if ( ( $this->mLastResult = $stmt = oci_parse( $this->mConn, $sql ) ) === false ) {
305307 $e = oci_error( $this->mConn );
306308 $this->reportQueryError( $e['message'], $e['code'], $sql, __FUNCTION__ );
——@@ -465,7 +467,7 @@
466468 } else {
467469 $sql .= $val !== null ? ', :' . $col : ', NULL';
468470 }
469 -
 471+
470472 $first = false;
471473 }
472474 $sql .= ')';
——@@ -484,7 +486,7 @@
485487 if ( preg_match( '/^timestamp.*/i', $col_type ) == 1 && strtolower( $val ) == 'infinity' ) {
486488 $val = '31-12-2030 12:00:00.000000';
487489 }
488 -
 490+
489491 $val = ( $wgLang != null ) ? $wgLang->checkTitleEncoding( $val ) : $val;
490492 if ( oci_bind_by_name( $stmt, ":$col", $val ) === false ) {
491493 $this->reportQueryError( $this->lastErrno(), $this->lastError(), $sql, __METHOD__ );
——@@ -508,7 +510,7 @@
509511 $olderr = error_reporting( E_ERROR );
510512 if ( oci_execute( $stmt, OCI_DEFAULT ) === false ) {
511513 $e = oci_error( $stmt );
512 -
 514+
513515 if ( !$this->ignore_DUP_VAL_ON_INDEX || $e['code'] != '1' ) {
514516 $this->reportQueryError( $e['message'], $e['code'], $sql, __METHOD__ );
515517 } else {
——@@ -518,13 +520,13 @@
519521 $this->mAffectedRows = oci_num_rows( $stmt );
520522 }
521523 error_reporting( $olderr );
522 -
 524+
523525 if ( isset( $lob ) ) {
524526 foreach ( $lob as $lob_i => $lob_v ) {
525527 $lob_v->free();
526528 }
527529 }
528 -
 530+
529531 if ( !$this->mTrxLevel ) {
530532 oci_commit( $this->mConn );
531533 }
——@@ -545,17 +547,17 @@
546548 } else {
547549 $srcTable = $this->tableName( $srcTable );
548550 }
549 -
 551+
550552 if ( ( $sequenceData = $this->getSequenceData( $destTable ) ) !== false &&
551553 !isset( $varMap[$sequenceData['column']] ) )
552554 $varMap[$sequenceData['column']] = 'GET_SEQUENCE_VALUE(\'' . $sequenceData['sequence'] . '\')';
553 -
 555+
554556 // count-alias subselect fields to avoid abigious definition errors
555557 $i = 0;
556558 foreach ( $varMap as $key => &$val ) {
557559 $val = $val . ' field' . ( $i++ );
558560 }
559 -
 561+
560562 $sql = "INSERT INTO $destTable (" . implode( ',', array_keys( $varMap ) ) . ')' .
561563 " SELECT $startOpts " . implode( ',', $varMap ) .
562564 " FROM $srcTable $useIndex ";
——@@ -563,13 +565,13 @@
564566 $sql .= ' WHERE ' . $this->makeList( $conds, LIST_AND );
565567 }
566568 $sql .= " $tailOpts";
567 -
 569+
568570 if ( in_array( 'IGNORE', $insertOptions ) ) {
569571 $this->ignore_DUP_VAL_ON_INDEX = true;
570572 }
571 -
 573+
572574 $retval = $this->query( $sql, $fname );
573 -
 575+
574576 if ( in_array( 'IGNORE', $insertOptions ) ) {
575577 $this->ignore_DUP_VAL_ON_INDEX = false;
576578 }
——@@ -581,7 +583,7 @@
582584 global $wgSharedDB, $wgSharedPrefix, $wgSharedTables;
583585 /*
584586 Replace reserved words with better ones
585 - Using uppercase because that's the only way Oracle can handle
 587+ Using uppercase because that's the only way Oracle can handle
586588 quoted tablenames
587589 */
588590 switch( $name ) {
——@@ -612,7 +614,7 @@
613615 }
614616
615617 $prefix = $this->mTablePrefix;
616 -
 618+
617619 if ( isset( $database ) ) {
618620 $table = ( $table[0] == '`' ? $table : "`{$table}`" );
619621 }
——@@ -653,7 +655,7 @@
654656 function getSequenceData( $table ) {
655657 if ( $this->sequenceData == null ) {
656658 $result = $this->query( "SELECT lower(us.sequence_name), lower(utc.table_name), lower(utc.column_name) from user_sequences us, user_tab_columns utc where us.sequence_name = utc.table_name||'_'||utc.column_name||'_SEQ'" );
657 -
 659+
658660 while ( ( $row = $result->fetchRow() ) !== false ) {
659661 $this->sequenceData[$this->tableName( $row[1] )] = array(
660662 'sequence' => $row[0],
——@@ -664,7 +666,7 @@
665667
666668 return ( isset( $this->sequenceData[$table] ) ) ? $this->sequenceData[$table] : false;
667669 }
668 -
 670+
669671 # REPLACE query wrapper
670672 # Oracle simulates this with a DELETE followed by INSERT
671673 # $row is the row to insert, an associative array
——@@ -851,7 +853,11 @@
852854 * based on prebuilt table to simulate MySQL field info and keep query speed minimal
853855 */
854856 function fieldExists( $table, $field, $fname = 'DatabaseOracle::fieldExists' ) {
855 - if ( !isset( $this->fieldInfo_stmt ) ) {
 857+ $table = trim( $table, '"' );
 858+
 859+ if (isset($this->mFileInfoCache[$table.'.'.$field])) {
 860+ return true;
 861+ } elseif ( !isset( $this->fieldInfo_stmt ) ) {
856862 $this->fieldInfo_stmt = oci_parse( $this->mConn, 'SELECT * FROM wiki_field_info_full WHERE table_name = upper(:tab) and column_name = UPPER(:col)' );
857863 }
858864
——@@ -864,15 +870,23 @@
865871 return false;
866872 }
867873 $res = new ORAResult( $this, $this->fieldInfo_stmt );
868 - return $res->numRows() != 0;
 874+ if ($res->numRows() != 0) {
 875+ $this->mFileInfoCache[$table.'.'.$field] = new ORAField( $res->fetchRow() );
 876+ return true;
 877+ } else {
 878+ return false;
 879+ }
869880 }
870881
871882 function fieldInfo( $table, $field ) {
872 - if ( !isset( $this->fieldInfo_stmt ) ) {
 883+ $table = trim( $table, '"' );
 884+
 885+ if (isset($this->mFileInfoCache[$table.'.'.$field])) {
 886+ return $this->mFileInfoCache[$table.'.'.$field];
 887+ } elseif ( !isset( $this->fieldInfo_stmt ) ) {
873888 $this->fieldInfo_stmt = oci_parse( $this->mConn, 'SELECT * FROM wiki_field_info_full WHERE table_name = upper(:tab) and column_name = UPPER(:col)' );
874889 }
875890
876 - $table = trim( $table, '"' );
877891 oci_bind_by_name( $this->fieldInfo_stmt, ':tab', $table );
878892 oci_bind_by_name( $this->fieldInfo_stmt, ':col', $field );
879893
——@@ -882,7 +896,8 @@
883897 return false;
884898 }
885899 $res = new ORAResult( $this, $this->fieldInfo_stmt );
886 - return new ORAField( $res->fetchRow() );
 900+ $this->mFileInfoCache[$table.'.'.$field] = new ORAField( $res->fetchRow() );
 901+ return $this->mFileInfoCache[$table.'.'.$field];
887902 }
888903
889904 function begin( $fname = '' ) {
——@@ -908,7 +923,7 @@
909924 $cmd = '';
910925 $done = false;
911926 $dollarquote = false;
912 -
 927+
913928 $replacements = array();
914929
915930 while ( ! feof( $fp ) ) {
——@@ -1155,7 +1170,7 @@
11561171 function getServer() {
11571172 return $this->mServer;
11581173 }
1159 -
 1174+
11601175 public function replaceVars( $ins ) {
11611176 $varnames = array( 'wgDBprefix' );
11621177 if ( $this->mFlags & DBO_SYSDBA ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r60730Fixed variable name typo from r60665.freakolowsky13:52, 6 January 2010
r60731Fixed variable name typo from r60665. (problems while sending file in previou...freakolowsky14:00, 6 January 2010
r61296Fixed as per Tim's comments on r60665:...freakolowsky16:19, 20 January 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r58356overloaded makeList in DatabaseOracle to handle CLOBs in WHERE clausesfreakolowsky09:44, 30 October 2009

Comments

#Comment by Bryan (talk | contribs)   15:46, 5 January 2010

It's nice to state in the commit message not only that it fixes a certain revision, but also what it fixes, e.g. "Fix for r58356: Cache file info for DatabaseOracle"

#Comment by 😂 (talk | contribs)   13:43, 6 January 2010

Yes, and also please keep whitespace changes separate from functional changes.

#Comment by MaxSem (talk | contribs)   17:39, 5 January 2010

Shouldn't that var be called mFieldInfoCache instead of mFileInfoCache? :)

#Comment by Freakolowsky (talk | contribs)   13:55, 6 January 2010

fixed in r60730 (except for whitespace comment ... don't have a clue where that came from)

#Comment by Tim Starling (talk | contribs)   03:13, 20 January 2010

There's code duplication here. DatabaseOracle::fieldExists() could just be implemented as: 

return (bool)$this->fieldInfo( $table, $field );

and that would save a lot of spot-the-difference and maintenance hassle. fieldInfo() would have to return false on non-existence instead of throwing a lot of warnings like it does now, but that's the documented behaviour in the parent interface so you should be doing that anyway. The cache is not really functional for fieldExists(), since you omit a negative cache, that is: 

for ( $i = 0; $i < 100; $i++ ) {
    $db->fieldExists( 'foo', 'bar' );
}

will generate 100 queries. It's a one-line fix (after refactoring).

Also there seems to be a case-folding issue. It may make sense to normalize the case of the table and field names before you construct the cache key, to avoid multiple queries in cases such as: 

$db->fieldExists( 'foo', 'bar' );
$db->fieldExists( 'foo', 'BAR' );
$db->fieldExists( 'foo', 'Bar' );

Then you could leave out the UPPER() calls in the queries.

#Comment by Freakolowsky (talk | contribs)   16:32, 20 January 2010

Fixed in r61296 ... hope it's ql now.

#Comment by Freakolowsky (talk | contribs)   11:43, 14 February 2010

I'm unable to change status ... this should have ok or resolved status

Status & tagging log