r71662 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r71661‎ | r71662 | r71663 >
Date:21:46, 25 August 2010
Author:reedy
Status:reverted (Comments)
Tags:
Comment:
Fill out insertOrUpdate in DatabaseBase, rather than blank stub. Followup to r71634
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/Database.php
——@@ -1164,15 +1164,49 @@
11651165 * @param $table String: table name (prefix auto-added)
11661166 * @param $a Array: Array of rows to insert
11671167 * @param $fname String: Calling function name (use __METHOD__) for logs/profiling
1168 - * @param $options Mixed: Associative array of options
 1168+ * @param $options Mixed: Associative array of options (ignored)
11691169 * @param $onDupeUpdate Array: Associative array of fields to update on duplicate
11701170 *
11711171 * @return bool
11721172 */
11731173 function insertOrUpdate( $table, $a, $fname = 'DatabaseBase::insertOnDupeUpdate', $options = array(), $onDupeUpdate = array() ) {
1174 - //TODO:FIXME
1175 - //$this->select();
1176 - //$this->replace();
 1174+
 1175+ if ( isset( $a[0] ) && is_array( $a[0] ) ) {
 1176+ $keys = array_keys( $a[0] );
 1177+ } else {
 1178+ $keys = array_keys( $a );
 1179+ }
 1180+
 1181+ //Get what is only to be set if inserted
 1182+ $where = array_diff( $a, $onDupeUpdate );
 1183+
 1184+ $res = $this->select(
 1185+ $table,
 1186+ $keys,
 1187+ $this->makeList( $where, LIST_AND ),
 1188+ __METHOD__
 1189+ );
 1190+
 1191+ if ( $res ) {
 1192+ //Where there is a different value to set if this is being "updated", use the $onDupeUpdate value for that to
 1193+ //replace the original option (if it was an insert), and replace the column name with the value read from
 1194+ //the existing row
 1195+ foreach( $where as $k => $v ){
 1196+ if ( isset( $onDupeUpdate[$k] ) ){
 1197+ $options[$k] = str_replace( $k, $res[0]->{$k}, $onDupeUpdate[$k] );
 1198+ }
 1199+ }
 1200+ } else {
 1201+ //No results, it's just an insert
 1202+ $update = $where;
 1203+ }
 1204+
 1205+ return (bool)$this->replace(
 1206+ $table,
 1207+ $update,
 1208+ array(),
 1209+ __METHOD__
 1210+ );
11771211 }
11781212
11791213 /**
——@@ -1660,6 +1694,9 @@
16611695 }
16621696 $sql .= '(' . $this->makeList( $row ) . ')';
16631697 }
 1698+ if ($fname === 'insertOnDupeUpdate') {
 1699+ var_dump($sql); die();
 1700+ }
16641701 return $this->query( $sql, $fname );
16651702 }
16661703

Follow-up revisions

RevisionCommit summaryAuthorDate
r71697Followup r71662, remove debug codereedy14:14, 26 August 2010
r71757Followup r71662...reedy22:23, 26 August 2010
r71758Drop $options from insertOrUpdate - r71662reedy22:31, 26 August 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r71634Move insertOnDupeUpdate (and rename to insertOrUpdate) to DatabaseMysql. Add ...reedy17:45, 25 August 2010

Comments

#Comment by Nikerabbit (talk | contribs)   13:55, 26 August 2010

var_dump?

#Comment by X! (talk | contribs)   14:00, 26 August 2010

Your debugging code is... well... not right. :)

#Comment by Simetrical (talk | contribs)   22:14, 26 August 2010

Add a FIXME comment here -- this doesn't properly implement MySQL's functionality. The rows could change in between the select and replace, at the very least. Actually, I don't understand how this is supposed to work, from reading the code . . . have you tested it? Better function-level comments would help, maybe with sample usage to show the format of $onDupeUpdate at least.

Also: 

  	function insertOrUpdate( $table, $a, $fname = 'DatabaseBase::insertOnDupeUpdate', $options = array(), $onDupeUpdate = array() ) {

You need to change the default $fname. 

+		//Get what is only to be set if inserted

Put a space after the "//" (here and other places). 

+			foreach( $where as $k => $v ){

Put a space in "){" (here and another place). 

+	 * @param $options Mixed: Associative array of options (ignored)

So why not remove the param? Didn't you just add this method now? 

+			$this->makeList( $where, LIST_AND ),

Surely you can just pass $where here directly, with no makeList()?


#Comment by Reedy (talk | contribs)   22:40, 26 August 2010

This was an rough implementation to give the "same" functionality as that of INSERT...ON DUPLICATE KEY UPDATE (see DatabaseMysql::insertOrUpdate)

For the locking, you're right. I can put in a "FOR UPDATE" option for the base implemenation. The usage seems hit and miss (Sqllite for example, just removes the option locally)


The function was for, at least, in MySQL, to prevent the read write cycle. Elsewhere, this is seemingly needed. In the INSERT...ON DUPLICATE KEY UPDATE, we can use a reference to the column being updated, and use that value, to be updated. This pulls these values that are to be updated (PK stuff in the mysql one would stay the same). It then pulls these values into col names in the keys of the stuff from onDupeUpdate

#Comment by Simetrical (talk | contribs)   16:43, 27 August 2010

I really still cannot understand what exactly $onDupeUpdate is supposed to do. Could you please give usage examples, both for single-row and multi-row cases, and say what they're expected to do? Assuming you don't just remove this entirely, as I suggested in r71609.

#Comment by Reedy (talk | contribs)   16:57, 27 August 2010

Hmm, why am I using replace, not Update? I'm not sure on that extent...

Multi row would be weird (I did ask about just doing one row, but Roan said to leave it)

For the MySQL version: INSERT INTO $table( $col1, $col2, $col3, $col4 ) VALUES( 1, 2, 3, 4 ) ON DUPLICATE KEY UPDATE $col4 = $col4 + 4;

There is then stuff you only want to insert if this is a new row (PK stuff etc).

The onDupeUpdate array, is for the stuff that only wants updating if it already exists. This has the current value from the select (whereas it's one statement in MySQL), replaces that in the key of onDupeUpdate

So it becomes something along the lines of

UPDATE $table SET $col4 = 4 + 4 WHERE $col1 = 1;


Status & tagging log