Re: bzr commit into mysql-6.0-falcon-team branch (kevin.lewis:3015) Bug#34182

[Olav Sandstaa]

Hi Kevin,

I really like the simple solution you managed to make for solving this 
fairly complex problem.

I have only some questions/comments to the test cases:

* I think we try to avoid adding "engine falcon" to create statements 
since we have already set default engine in our tests?

* Test case 1: will this test fail if the second update (which is 
supposed to block) do not block? Would it be possible to show that it 
actually blocks?

* Test case 1: would it be good to add a "Check results for test 1" 
section that did a select * from the tables?

* Test case 2: Here you do a "Connect to default and ROLLBACK TO SP1" 
(but issue a ROLLBACK command?) but this seems like both the initially 
"waiting" updates have gone through? both 1111 and 2222 have been 
updated. Is this correct?

* Test case 3: will this test fail in anyway iof the first update does 
not block? Would it be possible to show that it actually blocks?

The actual code for the implementation looks correct.

Given that you do not see any issues when reading my comments to the 
test case, the patch is OK to push!

Olav


Kevin Lewis wrote:
> #At file:///C:/Work/bzr/Merge/mysql-6.0-falcon-team/ based on
> revid:kevin.lewis@stripped
>
>  3015 Kevin Lewis	2009-02-12
>       Bug#34182 - Falcon needs to be able to keep locks at lower savepoints when they
> are explicitly released or uinlocked at hight savepoints  Locks should be retained during
> the life of that savepoint unless they are explicitely unlocked before the next savepoint.
> 
>       
>       It is not necessary to add a new lock record at each savepoint.  The existing
> lock will do the job as long as it is not unlocked at a later savepoint.  The best
> solution to this problem is to only unlock records that were locked in the current
> savepoint, or verb.
>       
>       A new integer is sent into unlockRecords which holds the current verbMark or
>       savepoint.  LockRecords that are at a higher savepoint are not unlocked.  Then,
> if a
>       commit, rollback, releaseSavepoint, or rollbackToSavepoint occurs that would
> release that
>       lockRecord, it will get released just like it did before.  
>       
>       A new testcase is added  that shows the affect of locks attained at multiple
> savepoints.
> added:
>   mysql-test/suite/falcon/r/falcon_bug_34182.result
>   mysql-test/suite/falcon/t/falcon_bug_34182.test
> modified:
>   storage/falcon/StorageTable.cpp
>   storage/falcon/Table.cpp
>   storage/falcon/Table.h
>   storage/falcon/Transaction.cpp
>
> === added file 'mysql-test/suite/falcon/r/falcon_bug_34182.result'
> --- a/mysql-test/suite/falcon/r/falcon_bug_34182.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/falcon/r/falcon_bug_34182.result	2009-02-12 20:22:40 +0000
> @@ -0,0 +1,141 @@
> +*** Bug #34182 ***
> +# Initialisation
> +SET @@storage_engine = 'Falcon';
> +DROP TABLE IF EXISTS t1;
> +DROP TABLE IF EXISTS t2;
> +# Test #1 - Double locking in a subselect
> +CREATE TABLE t1 (id INTEGER primary key, x INTEGER) ENGINE falcon;
> +CREATE TABLE t2 (b integer, a integer) ENGINE falcon;
> +INSERT INTO t1 VALUES (1, 1), (222, 222);
> +INSERT INTO t2 VALUES (1, 10), (2, 20), (3, 30);
> +SELECT * FROM t1;
> +id	x
> +1	1
> +222	222
> +SELECT * FROM t2;
> +b	a
> +1	10
> +2	20
> +3	30
> +# Following UPDATE should leave record id=1 locked in T1.
> +BEGIN;
> +UPDATE t2 SET a = 100 WHERE b = (SELECT x FROM t1 WHERE id = b FOR UPDATE);
> +# Establish connection conn1 (user = root)
> +# conn1 should be able to make updates to t1 id=222
> +UPDATE t1 SET x = 2222 WHERE id = 222;
> +# conn1 should not be able to make updates to t1 id=1
> +UPDATE t1 SET x = 1111 WHERE id = 1;
> +# Connect to default and COMMIT
> +COMMIT;
> +# Connect to conn1 and reap the UPDATE
> +# Connect to default and prepare for Test #2
> +DROP TABLE t1;
> +DROP TABLE t2;
> +# Test #2 - Multiple locking within savepoints
> +CREATE TABLE t1 (id INTEGER primary key, x INTEGER) ENGINE falcon;
> +INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> +BEGIN;
> +SELECT * FROM t1 WHERE id = 1 FOR UPDATE;
> +id	x
> +1	1
> +SAVEPOINT SP1;
> +SELECT * FROM t1 WHERE id <= 2 FOR UPDATE;
> +id	x
> +1	1
> +2	2
> +SAVEPOINT SP2;
> +SELECT * FROM t1 WHERE id <= 3 FOR UPDATE;
> +id	x
> +1	1
> +2	2
> +3	3
> +# This update will fetch all 3 records for update, 
> +# replace the lock on id=3, and then attempt to unlock
> +# both id=1 and id=2.  But they should remain locked.
> +UPDATE t1 SET x = 333 WHERE x = 3;
> +SELECT * FROM t1;
> +id	x
> +1	1
> +2	2
> +3	333
> +# conn1 should not be able to make updates to t1 id=1
> +UPDATE t1 SET x = 1111 WHERE id = 1;
> +# Establish connection conn2 (user = root)
> +# conn2 should not be able to make updates to t1 id=2
> +UPDATE t1 SET x = 2222 WHERE id = 2;
> +# Connect to default and ROLLBACK TO SP1
> +ROLLBACK;
> +SELECT * FROM t1;
> +id	x
> +1	1111
> +2	2222
> +3	3
> +# Check Results For Test 2
> +# Check Results
> +# Connect to conn1 and reap
> +SELECT * FROM t1;
> +id	x
> +1	1111
> +2	2222
> +3	3
> +# Connect to conn2 and reap
> +SELECT * FROM t1;
> +id	x
> +1	1111
> +2	2222
> +3	3
> +# Test #3 - Multiple locking within savepoints
> +# Connect to default and lock records again
> +BEGIN;
> +SELECT * FROM t1 WHERE id = 1 FOR UPDATE;
> +id	x
> +1	1111
> +SAVEPOINT SP1;
> +SELECT * FROM t1 WHERE id <= 2 FOR UPDATE;
> +id	x
> +1	1111
> +2	2222
> +SAVEPOINT SP2;
> +SELECT * FROM t1 WHERE id <= 3 FOR UPDATE;
> +id	x
> +1	1111
> +2	2222
> +3	3
> +UPDATE t1 SET x = 333 WHERE x = 3;
> +SELECT * FROM t1;
> +id	x
> +1	1111
> +2	2222
> +3	333
> +ROLLBACK TO SP1;
> +SELECT * FROM t1;
> +id	x
> +1	1111
> +2	2222
> +3	3
> +# conn1 should not be able to make updates to t1 id=1
> +UPDATE t1 SET x = 111111 WHERE id = 1;
> +# conn2 should be able to make updates to t1 id=2
> +UPDATE t1 SET x = 222222 WHERE id = 2;
> +# Connect to default and commit;
> +COMMIT;
> +SELECT * FROM t1;
> +id	x
> +1	111111
> +2	222222
> +3	3
> +# Check Results
> +# Connect to conn1 and reap
> +SELECT * FROM t1;
> +id	x
> +1	111111
> +2	222222
> +3	3
> +# Connect to conn2
> +SELECT * FROM t1;
> +id	x
> +1	111111
> +2	222222
> +3	3
> +# Final cleanup
> +DROP TABLE t1;
>
> === added file 'mysql-test/suite/falcon/t/falcon_bug_34182.test'
> --- a/mysql-test/suite/falcon/t/falcon_bug_34182.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/falcon/t/falcon_bug_34182.test	2009-02-12 20:22:40 +0000
> @@ -0,0 +1,169 @@
> +--source include/have_falcon.inc
> +#--disable_abort_on_error
> +#
> +# Bug #34182: SELECT ... FOR UPDATE does not lock when in subquery
> +#
> +--echo *** Bug #34182 ***
> +
> +# ----------------------------------------------------- #
> +# --- Initialisation                                --- #
> +# ----------------------------------------------------- #
> +--echo # Initialisation
> +let $engine = 'Falcon';
> +eval SET @@storage_engine = $engine;
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +DROP TABLE IF EXISTS t2;
> +--enable_warnings
> +
> +# ----------------------------------------------------- #
> +# --- Test #1 - Double locking in a subselect       --- #
> +# ----------------------------------------------------- #
> +--echo # Test #1 - Double locking in a subselect
> +CREATE TABLE t1 (id INTEGER primary key, x INTEGER) ENGINE falcon;
> +CREATE TABLE t2 (b integer, a integer) ENGINE falcon;
> +INSERT INTO t1 VALUES (1, 1), (222, 222);
> +INSERT INTO t2 VALUES (1, 10), (2, 20), (3, 30);
> +SELECT * FROM t1;
> +SELECT * FROM t2;
> +
> +--echo # Following UPDATE should leave record id=1 locked in T1.
> +BEGIN;
> +UPDATE t2 SET a = 100 WHERE b = (SELECT x FROM t1 WHERE id = b FOR UPDATE);
> +
> +
> +--echo # Establish connection conn1 (user = root)
> +connect (conn1,localhost,root,,);
> +connection conn1;
> +
> +--echo # conn1 should be able to make updates to t1 id=222
> +UPDATE t1 SET x = 2222 WHERE id = 222;  
> +
> +--echo # conn1 should not be able to make updates to t1 id=1
> +--send  UPDATE t1 SET x = 1111 WHERE id = 1
> +
> +--echo # Connect to default and COMMIT
> +connection default;
> +--real_sleep 1
> +COMMIT;
> +
> +--echo # Connect to conn1 and reap the UPDATE
> +connection conn1;
> +--reap
> +--echo # Connect to default and prepare for Test #2
> +connection default;
> +DROP TABLE t1;
> +DROP TABLE t2;
> +
> +# ----------------------------------------------------- #
> +# --- Test #2 - Multiple locking within savepoints  --- #
> +# ----------------------------------------------------- #
> +--echo # Test #2 - Multiple locking within savepoints
> +CREATE TABLE t1 (id INTEGER primary key, x INTEGER) ENGINE falcon;
> +INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> +BEGIN;
> +SELECT * FROM t1 WHERE id = 1 FOR UPDATE;
> +SAVEPOINT SP1;
> +SELECT * FROM t1 WHERE id <= 2 FOR UPDATE;
> +SAVEPOINT SP2;
> +SELECT * FROM t1 WHERE id <= 3 FOR UPDATE;
> +
> +--echo # This update will fetch all 3 records for update, 
> +--echo # replace the lock on id=3, and then attempt to unlock
> +--echo # both id=1 and id=2.  But they should remain locked.
> +UPDATE t1 SET x = 333 WHERE x = 3;
> +SELECT * FROM t1;
> +
> +
> +--echo # conn1 should not be able to make updates to t1 id=1
> +connection conn1;
> +--send  UPDATE t1 SET x = 1111 WHERE id = 1
> +
> +--echo # Establish connection conn2 (user = root)
> +connect (conn2,localhost,root,,);
> +connection conn2;
> +--echo # conn2 should not be able to make updates to t1 id=2
> +--send  UPDATE t1 SET x = 2222 WHERE id = 2
> +
> +--echo # Connect to default and ROLLBACK TO SP1
> +connection default;
> +--real_sleep 1
> +ROLLBACK;
> +--real_sleep 1
> +SELECT * FROM t1;
> +
> +# ----------------------------------------------------- #
> +# --- Check Results For Test 2                      --- #
> +# ----------------------------------------------------- #
> +--echo # Check Results For Test 2
> +--echo # Check Results
> +--echo # Connect to conn1 and reap
> +connection conn1;
> +--reap
> +SELECT * FROM t1;
> +
> +--echo # Connect to conn2 and reap
> +connection conn2;
> +--reap
> +SELECT * FROM t1;
> +
> +
> +# ----------------------------------------------------- #
> +# --- Test #3 - More locking within savepoints      --- #
> +# ----------------------------------------------------- #
> +--echo # Test #3 - Multiple locking within savepoints
> +--echo # Connect to default and lock records again
> +connection default;
> +BEGIN;
> +SELECT * FROM t1 WHERE id = 1 FOR UPDATE;
> +SAVEPOINT SP1;
> +SELECT * FROM t1 WHERE id <= 2 FOR UPDATE;
> +SAVEPOINT SP2;
> +SELECT * FROM t1 WHERE id <= 3 FOR UPDATE;
> +UPDATE t1 SET x = 333 WHERE x = 3;
> +SELECT * FROM t1;
> +ROLLBACK TO SP1;
> +SELECT * FROM t1;
> +
> +
> +--echo # conn1 should not be able to make updates to t1 id=1
> +connection conn1;
> +--send  UPDATE t1 SET x = 111111 WHERE id = 1
> +
> +--echo # conn2 should be able to make updates to t1 id=2
> +connection conn2;
> +UPDATE t1 SET x = 222222 WHERE id = 2;
> +
> +
> +--echo # Connect to default and commit;
> +connection default;
> +--real_sleep 1
> +COMMIT;
> +--real_sleep 1
> +SELECT * FROM t1;
> +
> +
> +# ----------------------------------------------------- #
> +# --- Check Results For Test 3                      --- #
> +# ----------------------------------------------------- #
> +--echo # Check Results
> +--echo # Connect to conn1 and reap
> +connection conn1;
> +--reap
> +SELECT * FROM t1;
> +
> +--echo # Connect to conn2
> +connection conn2;
> +SELECT * FROM t1;
> +
> +# ----------------------------------------------------- #
> +# --- Final cleanup                                 --- #
> +# ----------------------------------------------------- #
> +--echo # Final cleanup
> +connection default;
> +disconnect conn1;
> +disconnect conn2;
> +
> +DROP TABLE t1;
> +
>
> === modified file 'storage/falcon/StorageTable.cpp'
> --- a/storage/falcon/StorageTable.cpp	2009-01-15 20:29:54 +0000
> +++ b/storage/falcon/StorageTable.cpp	2009-02-12 20:22:40 +0000
> @@ -628,7 +628,7 @@ void StorageTable::unlockRow(void)
>  {
>  	if (recordLocked)
>  		{
> -		share->table->unlockRecord(record->recordNumber);
> +		share->table->unlockRecord(record->recordNumber,
> storageConnection->verbMark);
>  		recordLocked = false;
>  		}
>  }
>
> === modified file 'storage/falcon/Table.cpp'
> --- a/storage/falcon/Table.cpp	2009-02-12 19:31:23 +0000
> +++ b/storage/falcon/Table.cpp	2009-02-12 20:22:40 +0000
> @@ -3393,34 +3393,39 @@ void Table::waitForWriteComplete()
>  	database->waitForWriteComplete(this);
>  }
>  
> -void Table::unlockRecord(int recordNumber)
> +void Table::unlockRecord(int recordNumber, int verbMark)
>  {
>  	Record *record = fetch(recordNumber);
>  
>  	if (record)
>  		{
>  		if (record->state == recLock)
> -			unlockRecord((RecordVersion*) record);
> -		
> +			unlockRecord((RecordVersion*) record, verbMark);
> +
>  		record->release();
>  		}
>  }
>  
> -void Table::unlockRecord(RecordVersion* record)
> +void Table::unlockRecord(RecordVersion* record, int verbMark)
>  {
> -	//int uc = record->useCount;
> -	ASSERT(record->getPriorVersion());
> +	if (record->state != recLock)
> +		return;
>  
>  	// A lock record that has superceded=true is already unlocked
>  
> -	if ((record->state == recLock) && !record->isSuperceded())
> -		{
> -		Record *prior = record->getPriorVersion();
> -		if (insertIntoTree(prior, record, record->recordNumber))
> -			record->setSuperceded(true);
> -		else
> -			Log::debug("Table::unlockRecord: record lock not in record tree\n");
> -		}
> +	if (record->isSuperceded())
> +		return;
> +
> +	// Only unlock records at the current savepoint
> +
> +	if (record->savePointId < verbMark)
> +		return;
> +
> +	Record *prior = record->getPriorVersion();
> +	if (insertIntoTree(prior, record, record->recordNumber))
> +		record->setSuperceded(true);
> +	else
> +		Log::debug("Table::unlockRecord: record lock not in record tree\n");
>  }
>  
>  void Table::checkAncestor(Record* current, Record* oldRecord)
>
> === modified file 'storage/falcon/Table.h'
> --- a/storage/falcon/Table.h	2009-02-09 05:01:44 +0000
> +++ b/storage/falcon/Table.h	2009-02-12 20:22:40 +0000
> @@ -211,8 +211,8 @@ public:
>  	Record*			allocRecord(int recordNumber, Stream* stream);
>  	Format*			getCurrentFormat(void);
>  	Record*			fetchForUpdate(Transaction* transaction, Record* record, bool
> usingIndex);
> -	void			unlockRecord(int recordNumber);
> -	void			unlockRecord(RecordVersion* record);
> +	void			unlockRecord(int recordNumber, int verbMark);
> +	void			unlockRecord(RecordVersion* record, int verbMark);
>  
>  	void			insert (Transaction *transaction, int count, Field **fields, Value
> **values);
>  	uint			insert (Transaction *transaction, Stream *stream);
>
> === modified file 'storage/falcon/Transaction.cpp'
> --- a/storage/falcon/Transaction.cpp	2009-01-09 21:49:16 +0000
> +++ b/storage/falcon/Transaction.cpp	2009-02-12 20:22:40 +0000
> @@ -396,7 +396,7 @@ void Transaction::rollback()
>  		record->nextInTrans = NULL;
>  
>  		if (record->state == recLock)
> -			record->format->table->unlockRecord(record);
> +			record->format->table->unlockRecord(record, 0);
>  		else
>  			record->rollback(this);
>  		
> @@ -1304,7 +1304,7 @@ void Transaction::releaseRecordLocks(voi
>  	for (RecordVersion *record = firstRecord; record; record = record->nextInTrans)
>  		if (record->state == recLock)
>  			{
> -			record->format->table->unlockRecord(record);
> +			record->format->table->unlockRecord(record, 0);
>  
>  			// Don't do  removeRecord(record); now.  Other threads might be 
>  			// pointing to it and need the transaction pointer to determine 
>
>
>