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

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) { >> >> > > -- Disclaimer: This message (including any attachments) contains confidential information intended for a specific individual and purpose, and is protected by law. If you are not the intended recipient, you should delete this message. Any disclosure, copying, or distribution of this message, or the taking of any action based on it, is strictly prohibited.