bzr commit into mysql-6.0-falcon-team branch (svoj:2946) Bug#34182

[Sergey Vojtovich]

#At file:///home/svoj/devel/bzr-mysql/mysql-6.0-falcon-team-bug34182/

 2946 Sergey Vojtovich	2008-12-19
      BUG#34182 - SELECT ... FOR UPDATE does not lock when in subquery
      
      Executing SELECT ... FOR UPDATE multiple times may release
      row locks set by previous queries. Affects not only subqueries,
      as was initially discovered.
renamed:
  mysql-test/suite/falcon_team/r/falcon_deadlock.result =>
mysql-test/suite/falcon/r/falcon_deadlock.result
  mysql-test/suite/falcon_team/t/falcon_deadlock.test =>
mysql-test/suite/falcon/t/falcon_deadlock.test
modified:
  mysql-test/suite/falcon_team/t/test2bug.def
  storage/falcon/StorageDatabase.cpp
  storage/falcon/ha_falcon.cpp
  mysql-test/suite/falcon/r/falcon_deadlock.result
  mysql-test/suite/falcon/t/falcon_deadlock.test

per-file messages:
  mysql-test/suite/falcon/r/falcon_deadlock.result
    Fixed falcon_deadlock.test and moved it to falcon suite.
  mysql-test/suite/falcon/t/falcon_deadlock.test
    Fixed falcon_deadlock.test and moved it to falcon suite.
  mysql-test/suite/falcon_team/t/test2bug.def
    Moved falcon_deadlock.test to falcon suite.
  storage/falcon/StorageDatabase.cpp
    Fixed that Falcon was releasing rows, locked for update by
    previous queries.
  storage/falcon/ha_falcon.cpp
    Added a comment.
=== renamed file 'mysql-test/suite/falcon_team/r/falcon_deadlock.result' =>
'mysql-test/suite/falcon/r/falcon_deadlock.result'
--- a/mysql-test/suite/falcon_team/r/falcon_deadlock.result	2008-12-18 10:49:29 +0000
+++ b/mysql-test/suite/falcon/r/falcon_deadlock.result	2008-12-19 08:45:18 +0000
@@ -26,6 +26,7 @@ id	x
 0	1
 COMMIT;
 # Switch to connection conn2
+ERROR HY000: Record has changed since last read in table 't1'
 ROLLBACK;
 # Switch to connection conn1
 SELECT * FROM t1;
@@ -50,10 +51,10 @@ a	b
 0	10
 1	20
 2	30
-UPDATE t2 SET a = 100 WHERE b = (SELECT x FROM t1 WHERE id = b FOR UPDATE);
+UPDATE t2 SET b = 100 WHERE a = (SELECT x FROM t1 WHERE id = a FOR UPDATE);
 SELECT * FROM t2;
 a	b
-0	10
+0	100
 1	20
 2	30
 SELECT * FROM t1;
@@ -70,11 +71,12 @@ id	x
 300	300
 COMMIT;
 # Switch to connection conn2
+ERROR HY000: Record has changed since last read in table 't1'
 COMMIT;
 # Switch to connection conn1
 SELECT * FROM t1;
 id	x
-0	2
+0	1
 300	300
 COMMIT;
 DROP TABLE t1;
@@ -121,11 +123,12 @@ id	x
 300	300
 COMMIT;
 # Switch to connection conn2
+ERROR HY000: Record has changed since last read in table 't1'
 COMMIT;
 # Switch to connection conn1
 SELECT * FROM t1;
 id	x
-0	2
+0	1
 300	300
 COMMIT;
 SELECT count(*) FROM t1;

=== renamed file 'mysql-test/suite/falcon_team/t/falcon_deadlock.test' =>
'mysql-test/suite/falcon/t/falcon_deadlock.test'
--- a/mysql-test/suite/falcon_team/t/falcon_deadlock.test	2008-12-18 10:49:29 +0000
+++ b/mysql-test/suite/falcon/t/falcon_deadlock.test	2008-12-19 08:45:18 +0000
@@ -98,7 +98,7 @@ INSERT INTO t2 VALUES (0, 10), (1, 20), 
 COMMIT;
 
 SELECT * FROM t2;
-UPDATE t2 SET a = 100 WHERE b = (SELECT x FROM t1 WHERE id = b FOR UPDATE);
+UPDATE t2 SET b = 100 WHERE a = (SELECT x FROM t1 WHERE id = a FOR UPDATE);
 SELECT * FROM t2;
 SELECT * FROM t1;
 
@@ -117,6 +117,14 @@ COMMIT;
 
 --echo # Switch to connection conn2
 connection conn2;
+# If we set Falcon to
+#     falcon_consistent_read = off
+#
+# we should get a
+#     Record has changed since last read in table 't1'
+# here. Please note that falcon_consistent_read = off
+# is the default setting.
+--error ER_CHECKREAD
 --reap
 COMMIT;
 
@@ -166,6 +174,14 @@ COMMIT;
 
 --echo # Switch to connection conn2
 connection conn2;
+# If we set Falcon to
+#     falcon_consistent_read = off
+#
+# we should get a
+#     Record has changed since last read in table 't1'
+# here. Please note that falcon_consistent_read = off
+# is the default setting.
+--error ER_CHECKREAD
 --reap
 COMMIT;
 

=== modified file 'mysql-test/suite/falcon_team/t/test2bug.def'
--- a/mysql-test/suite/falcon_team/t/test2bug.def	2008-12-16 21:11:25 +0000
+++ b/mysql-test/suite/falcon_team/t/test2bug.def	2008-12-19 08:45:18 +0000
@@ -19,4 +19,3 @@
 
 falcon_bug_34174 : Bug#34174 - Infinite loop checking rolled back record in select for
update
 falcon_bug_36294-big: Bug#36631	- Assertion in SerialLogControl::nextRecord, sometimes.
-falcon_deadlock:  Bug#34182 - SELECT ... FOR UPDATE does not lock when in subquery

=== modified file 'storage/falcon/StorageDatabase.cpp'
--- a/storage/falcon/StorageDatabase.cpp	2008-10-16 02:53:35 +0000
+++ b/storage/falcon/StorageDatabase.cpp	2008-12-19 08:45:18 +0000
@@ -274,6 +274,7 @@ int StorageDatabase::nextRow(StorageTabl
 	Transaction *transaction = connection->getTransaction();
 	Record *candidate = NULL;
 	Record *record = NULL;
+	bool recordWasLocked;
 	
 	try
 		{
@@ -283,9 +284,25 @@ int StorageDatabase::nextRow(StorageTabl
 			if (!candidate)
 				return StorageErrorRecordNotFound;
 
-			record = (lockForUpdate)
-			               ? table->fetchForUpdate(transaction, candidate, false)
-			               : candidate->fetchVersion(transaction);
+			if (lockForUpdate)
+				{
+				// It may happen that the candidate was already locked by
+				// this transaction for update. Table::fetchForUpdate()
+				// doesn't lock a record in this case.
+				// 
+				// As no lock occured here, we must inform StorageTable::setRecord()
+				// about it by setting recordWasLocked to false. We do it by
+				// comparing candidate's transaction and current transaction.
+				// It must be done before we call Table::fetchForUpdate(), because
+				// it may release candidate.
+				recordWasLocked = (candidate->getTransaction() != transaction);
+				record = table->fetchForUpdate(transaction, candidate, false);
+				}
+				else
+				{
+				recordWasLocked = false;
+				record = candidate->fetchVersion(transaction);
+				}
 			
 			if (!record)
 				{
@@ -303,7 +320,7 @@ int StorageDatabase::nextRow(StorageTabl
 				}
 			
 			recordNumber = record->recordNumber;
-			storageTable->setRecord(record, lockForUpdate);
+			storageTable->setRecord(record, recordWasLocked);
 			
 			return recordNumber;
 			}
@@ -342,7 +359,9 @@ int StorageDatabase::fetch(StorageConnec
 	Connection *connection = storageConnection->connection;
 	Transaction *transaction = connection->getTransaction();
 	Record *candidate = NULL;;
-	
+	Record *record;
+	bool recordWasLocked;	
+
 	try
 		{
 		candidate = table->fetch(recordNumber);
@@ -350,9 +369,25 @@ int StorageDatabase::fetch(StorageConnec
 		if (!candidate)
 			return StorageErrorRecordNotFound;
 
-		Record *record = (lockForUpdate)
-		               ? table->fetchForUpdate(transaction, candidate, false)
-		               : candidate->fetchVersion(transaction);
+		if (lockForUpdate)
+			{
+			// It may happen that the candidate was already locked by
+			// this transaction for update. Table::fetchForUpdate()
+			// doesn't lock a record in this case.
+			// 
+			// As no lock occured here, we must inform StorageTable::setRecord()
+			// about it by setting recordWasLocked to false. We do it by
+			// comparing candidate's transaction and current transaction.
+			// It must be done before we call Table::fetchForUpdate(), because
+			// it may release candidate.
+			recordWasLocked = (candidate->getTransaction() != transaction);
+			record = table->fetchForUpdate(transaction, candidate, false);
+			}
+			else
+			{
+			recordWasLocked = false;
+			record = candidate->fetchVersion(transaction);
+			}
 		
 		if (!record)
 			{
@@ -368,7 +403,7 @@ int StorageDatabase::fetch(StorageConnec
 			candidate->release();
 			}
 		
-		storageTable->setRecord(record, lockForUpdate);
+		storageTable->setRecord(record, recordWasLocked);
 				
 		return 0;
 		}
@@ -425,9 +460,26 @@ int StorageDatabase::nextIndexed(Storage
 			
 			if (candidate)
 				{
-				Record *record = (lockForUpdate) 
-				               ? table->fetchForUpdate(transaction, candidate, true)
-				               : candidate->fetchVersion(transaction);
+				Record *record;
+				bool recordWasLocked;
+				if (lockForUpdate)
+					{
+					// It may happen that the candidate was already locked by
+					// this transaction for update. Table::fetchForUpdate()
+					// doesn't lock a record in this case.
+					// 
+					// As no lock occured here, we must inform StorageTable::setRecord()
+					// about it by setting recordWasLocked to false. We do it by
+					// comparing candidate's transaction and current transaction.
+					// It must be done before we call Table::fetchForUpdate(), because
+					// it may release candidate.					recordWasLocked = (candidate->getTransaction()
!= transaction);
+					record = table->fetchForUpdate(transaction, candidate, true);
+					}
+					else
+					{
+					recordWasLocked = false;
+					record = candidate->fetchVersion(transaction);
+					}
 				
 				if (record)
 					{
@@ -439,7 +491,7 @@ int StorageDatabase::nextIndexed(Storage
 						candidate->release();
 						}
 					
-					storageTable->setRecord(record, lockForUpdate);
+					storageTable->setRecord(record, recordWasLocked);
 					
 					return recordNumber;
 					}

=== modified file 'storage/falcon/ha_falcon.cpp'
--- a/storage/falcon/ha_falcon.cpp	2008-12-18 01:44:03 +0000
+++ b/storage/falcon/ha_falcon.cpp	2008-12-19 08:45:18 +0000
@@ -944,6 +944,22 @@ bool StorageInterface::check_if_incompat
 	return COMPATIBLE_DATA_YES;
 }
 
+// How does SELECT ... FOR UPDATE work:
+// The server is upgrading lock type for SELECT ... FOR UPDATE,
+// so here we get lock_type set to TL_WRITE.
+//
+// Falcon remembers that we got a write lock and sets lockForUpdate
+// accordingly. Note that we may get TL_WRITE with UPDATE query,
+// which may also lock the row during table scan, and we must still
+// lock the row in this case.
+//
+// When performing table/index scan underlying functions are expected
+// to lock fetched rows. It may happen that retrieved row doesn't match
+// a condition. In this case server calls StorageInterface::unlock_row(),
+// which is intended to release just locked row.
+//
+// Note that the row may be locked by previous table/index scans. In this
+// case ::unlock_row() must preserve the lock.
 THR_LOCK_DATA **StorageInterface::store_lock(THD *thd, THR_LOCK_DATA **to,
                                             enum thr_lock_type lock_type)
 {