Re: [Patch] DB_DataObject and Postgres INSERT race condition obtaining last key

I think this should code (or something similar) should really be in the getSequenceName() of MDB2 - Lukas may be able to shed some light on that. Matt Craig wrote: > The currval fix works in my test cases. > > I have related question about the attached patch. The patch is for DataObject to get currval > from the real "serial" postgres sequence instead of DB-created sequences, as long as > the key is > marked with a nextval(). > > Is there a more efficient was to find the key? > And > Justin mentioned that getting the sequences corrected in DB itself is a preferrable fix, but > pretending we don't have the skill or time to fix DB, too, how should DataObject handle > the > difficulties passed on to it from DB? (Namely, that it always uses sequences named > "<table>_seq".) > > Matt > > Alan Knowles wrote: > >> I've used the currval() fix - can you check this works and let me know. >> Thanks >> Alan >> >> Matt Craig wrote: >> >> >>> DB_DataObject has a race condition in the PostgreSQL insert() + last inserted key code. >>> >>> Currently, the actual INSERT INTO statement is completed and then later in the code the >>> sequence itself is checked to see what the last used value of the sequence was. In >>> between >>> those two steps it is very easy for another INSERT INTO statement for the same table in >>> a >>> different object to increment the sequence a second time before the first object even >>> checks >>> what the value of the sequence is. The end result is that both objects return the same >>> last >>> insert key. One of them incorrectly, of course. >>> >>> This code fixes the problem by recognizing autoincrement fields for PostgreSQL and >>> selecting a >>> next value from the sequence to insert, rather than letting the automatic nextval() >>> function >>> run. This ensures that the same key inserted into the table gets returned as the last >>> inserted >>> key. >>> >>> The final section of the patch is in the joinAdd function to allow auto joins where the >>> tables >>> in the join share a column name given as the parameter $joinCol. This is a feature, >>> rather >>> than a bug fix and could be dropped, but the pgsql INSERT code is, in my estimation, >>> essential. >>> >>> This code has been tested and in production for about 3 months on both a Postgres >>> 7.4.13 and >>> 8.0 installation. >>> >>> matt >>> >>> <ESC>:wq >>> >>> ------------------------------------------------------------------------ >>> >>> ? DataObject.diff >>> Index: DataObject.php >>> =================================================================== >>> RCS file: /repository/pear/DB_DataObject/DataObject.php,v >>> retrieving revision 1.419 >>> diff -u -r1.419 DataObject.php >>> --- DataObject.php 4 Sep 2006 02:53:20 -0000 1.419 >>> +++ DataObject.php 13 Oct 2006 12:40:07 -0000 >>> @@ -937,6 +937,44 @@ >>> } >>> $this->$key = $keyvalue; >>> } >>> + else if ($key && $useNative) { >>> + // postgresql has a race condition if you try to get the last_value from >>> + // a sequence in the middle of multiple transactions inserting into the >>> same table >>> + // so in this case we will select the nextval before doing the insert and >>> use it >>> + if($dbtype === 'pgsql') { >>> + // by this point we know this is a nextval field in postgres >>> + $defs = $DB->tableInfo($this->__table); >>> + foreach($defs as $def){ >>> + if($def['name'] == $key) { >>> + $f = rawurldecode($def['flags']); >>> + if (preg_match("/nextval\((.*)\)/i", $f, $seqs)) { >>> + // gotta put some quotes around this since the table >>> description is no helpful >>> + // I'd much rather use $seqs[0][ >>> + $seq = "nextval('{$seqs[1]}')"; >>> + } >>> + break; >>> + } >>> + } >>> + if (!$seq) { >>> + $seq = $DB->getSequenceName($this->__table ); >>> + } >>> + >>> + $pgsql_key = $DB->getOne("SELECT $seq"); >>> + if (PEAR::isError($pgsql_key)) { >>> + $this->raiseError("error in sequence: $seq"); >>> + return false; >>> + } >>> + else if(intval($pgsql_key)!=$pgsql_key){ >>> + // we have a real problem here >>> + $this->raiseError('postgres seq value is not an >>> integer'); >>> + return false; >>> + } >>> + $this->$key = $pgsql_key; >>> + } >>> + } >>> + >>> + >>> + >>> >>> >>> >>> @@ -944,7 +982,10 @@ >>> >>> // if we are using autoincrement - skip the column... >>> if ($key && ($k == $key) && $useNative) { >>> - continue; >>> + if($dbtype!=='pgsql'){ >>> + // we already fetched an key for pgsql and it needs to be added to >>> the fields=values >>> + continue; >>> + } >>> } >>> >>> >>> @@ -1064,15 +1105,8 @@ >>> break; >>> >>> case 'pgsql': >>> - if (!$seq) { >>> - $seq = $DB->getSequenceName($this->__table ); >>> - } >>> - $pgsql_key = $DB->getOne("SELECT last_value FROM >>> ".$seq); >>> - if (PEAR::isError($pgsql_key)) { >>> - $this->raiseError($r); >>> - return false; >>> - } >>> - $this->$key = $pgsql_key; >>> + // break right out of this. We got the sequence above and >>> we'll use that instance >>> + // TODO: this code should be removed entirely >>> break; >>> >>> case 'ifx': >>> @@ -2022,7 +2056,7 @@ >>> // technically postgres native here... >>> // we need to get the new improved tabledata sorted out first. >>> >>> - if ( in_array($dbtype , array( 'mysql', 'mysqli', >>> 'mssql', 'ifx')) && >>> + if ( in_array($dbtype , array( 'mysql', 'mysqli', >>> 'mssql', 'ifx', 'pgsql')) && >>> ($table[$usekey] & DB_DATAOBJECT_INT) && >>> isset($realkeys[$usekey]) && ($realkeys[$usekey] == >>> 'N') >>> ) { >>> @@ -3112,6 +3146,13 @@ >>> } >>> } >>> >>> + // finally if these two table have column names that match do a join by >>> default on them >>> + if($ofield === false && $joinCol) { >>> + $ofield = $joinCol; >>> + $tfield = $joinCol; >>> + } >>> + >>> + >>> /* did I find a conneciton between them? */ >>> >>> if ($ofield === false) { >>> >>> >>> >> > > > ------------------------------------------------------------------------ > > ? DB_DataObject/DataObject.diff > ? DB_DataObject/DataObject2.diff > Index: DB_DataObject/DataObject.php > =================================================================== > RCS file: /repository/pear/DB_DataObject/DataObject.php,v > retrieving revision 1.421 > diff -u -r1.421 DataObject.php > --- DB_DataObject/DataObject.php 16 Oct 2006 02:17:14 -0000 1.421 > +++ DB_DataObject/DataObject.php 16 Oct 2006 17:49:24 -0000 > @@ -1064,6 +1064,16 @@ > break; > > case 'pgsql': > + $defs = $DB->tableInfo($this->__table); > + foreach($defs as $def){ > + if($def['name'] == $key) { > + $f = rawurldecode($def['flags']); > + if (preg_match("/nextval\((.*)\)/i", $f, $seqs)) { > + $seq = $seqs[1]; > + } > + break; > + } > + } > if (!$seq) { > $seq = $DB->getSequenceName($this->__table ); > } > >