List:Commits« Previous MessageNext Message »
From:marko.makela Date:January 24 2011 1:01pm
Subject:bzr push into mysql-trunk-innodb branch (marko.makela:3453 to 3455) WL#5458
View as plain text  
 3455 Marko Mäkelä	2011-01-24
      Fix some things after WL#5458.
      
      lock_rec_validate_page(): Rename the misleading parameter
      have_lock_trx_sys_mutex to locked_lock_trx_sys. There is no
      trx_sys->mutex, it is trx_sys->lock.
      
      lock_sys_create(): Remove some commented-out code.
      
      lock_set_lock_and_trx_wait(): Add debug assertions. Assert that
      trx->mutex is being held. Previously, this constraint was being
      violated.
      
      lock_rec_create(): Add debug assertions. Protect
      lock_set_lock_and_trx_wait() with trx->mutex.
      
      lock_rec_add_to_queue(): Add a debug assertion on caller_owns_trx_mutex.
      
      lock_move_rec_list_end(): Remove unnecessary acquisition of trx->mutex
      around lock_reset_lock_and_trx_wait(). A running transaction cannot
      have lock waits; therefore the lock waits can safely be reset while
      the transaction is not running, while holding just lock_sys->mutex.
      
      lock_deadlock_recursive(): Protect
      trx->lock.was_chosen_as_deadlock_victim with trx->mutex.
      
      lock_rec_queue_validate(): Rename have_lock_trx_sys_mutex to
      locked_lock_trx_sys. Add debug assertions. Remove some duplicated code.

    modified:
      storage/innobase/lock/lock0lock.c
 3454 Marko Mäkelä	2011-01-24
      Non-functional change, cleaning up after WL#5458.
      
      Remove the trx_get_on_id() wrapper, because it is inherently unsafe.
      Rename trx_get_on_id_low() to trx_get_on_id(). Adjust callers.

    modified:
      storage/innobase/include/trx0sys.h
      storage/innobase/include/trx0sys.ic
      storage/innobase/row/row0vers.c
      storage/innobase/trx/trx0trx.c
 3453 Marko Mäkelä	2011-01-24
      Instead of checking if trx->state != some_expected_value, check
      trx->state against all allowed values, to detect corruption.
      
      trx_state_eq(trx, state): Test if trx->state == state. In debug builds,
      check trx->state, trx->in_mysql_trx_list, and trx->in_trx_list for corruption.
      
      trx_assert_started(): A debug predicate. Assert if trx is not in one
      of the active states.
      
      trx_in_trx_list(): Assert trx_was_started(). This allows us to remove
      the trx->state assertions in the caller.
      
      lock_table_queue_validate(), lock_rec_queue_validate(),
      lock_rec_validate_page(): Remove unnecessary acquisition of trx->mutex.
      Use trx_state_eq(), trx_was_started(), or trx_in_trx_list() for the
      trx->state checks.
      
      trx_is_active_low(), trx_recover_for_mysql(), trx_get_trx_by_xid(),
      read_view_open_now_low(), read_cursor_view_create_for_mysql(),
      row_vers_build_for_semi_consistent_read():
      Use trx_state_eq().
      
      trx_rollback_for_mysql(), trx_rollback_last_sql_stat_for_mysql(),
      trx_rollback_to_savepoint_for_mysql(), trx_rollback_resurrected(),
      trx_commit_or_rollback_prepare(), trx_mark_sql_stat_end():
      Check trx->state against all allowed values. Committed transactions
      must not be rolled back or committed. Partial rollback of prepared
      transactions is forbidden. Committed or prepared transactions must not
      execute statements.
      
      trx_rollback_to_savepoint_low(): Refactored from
      trx_rollback_to_savepoint_for_mysql(). Roll back to a savepoint
      identified by trx_named_savept_t.
      
      trx_release_savepoint_for_mysql(): Add debug assertions.
      
      trx_sys_init_at_db_start(): Use trx_state_eq(trx, TRX_STATE_ACTIVE).
      This might correct a bug where rows_to_undo would include the rows of
      recovered committed transactions.
      
      trx_list_insert_ordered(), trx_start_low(), trx_commit():
      Add debug assertions.
      
      rb://579 approved by Jimmy Yang

    modified:
      storage/innobase/include/trx0roll.h
      storage/innobase/include/trx0sys.h
      storage/innobase/include/trx0sys.ic
      storage/innobase/include/trx0trx.h
      storage/innobase/include/trx0trx.ic
      storage/innobase/lock/lock0lock.c
      storage/innobase/read/read0read.c
      storage/innobase/row/row0vers.c
      storage/innobase/trx/trx0roll.c
      storage/innobase/trx/trx0sys.c
      storage/innobase/trx/trx0trx.c
=== modified file 'storage/innobase/include/trx0sys.h'
--- a/storage/innobase/include/trx0sys.h	revid:marko.makela@strippedom-20110124102828-1o7l37nibd18jwja
+++ b/storage/innobase/include/trx0sys.h	revid:marko.makela@stripped125226-tecjgoda8urz5lkn
@@ -261,15 +261,10 @@ trx_read_trx_id(
 	const byte*	ptr);	/*!< in: pointer to memory from where to read */
 /****************************************************************//**
 Looks for the trx handle with the given id in trx_list.
-@return	the trx handle or NULL if not found */
-UNIV_INLINE
-trx_t*
-trx_get_on_id_low(
-/*==============*/
-	trx_id_t	trx_id);/*!< in: trx id to search for */
-/****************************************************************//**
-Looks for the trx handle with the given id in trx_list.
-@return	the trx handle or NULL if not found */
+The caller must be holding trx_sys->lock.
+@return	the trx handle or NULL if not found;
+the pointer must not be dereferenced unless lock_sys->mutex was
+acquired before calling this function and is still being held */
 UNIV_INLINE
 trx_t*
 trx_get_on_id(

=== modified file 'storage/innobase/include/trx0sys.ic'
--- a/storage/innobase/include/trx0sys.ic	revid:marko.makela@stripped
+++ b/storage/innobase/include/trx0sys.ic	revid:marko.makela@oracle.com-20110124125226-tecjgoda8urz5lkn
@@ -246,13 +246,15 @@ trx_read_trx_id(
 }
 
 /****************************************************************//**
-Looks for the trx handle with the given id in trx_list. Because the list
-is ordered on trx id in descending order, we try and speed things up a bit
-@return	the trx handle or NULL if not found */
+Looks for the trx handle with the given id in trx_list.
+The caller must be holding trx_sys->lock.
+@return	the trx handle or NULL if not found;
+the pointer must not be dereferenced unless lock_sys->mutex was
+acquired before calling this function and is still being held */
 UNIV_INLINE
 trx_t*
-trx_get_on_id_low(
-/*==============*/
+trx_get_on_id(
+/*==========*/
 	trx_id_t	trx_id)	/*!< in: trx id to search for */
 {
 	trx_t*		trx;
@@ -311,27 +313,7 @@ trx_get_on_id_low(
 }
 
 /****************************************************************//**
-Looks for the trx handle with the given id in trx_list.
-@return	the trx handle or NULL if not found */
-UNIV_INLINE
-trx_t*
-trx_get_on_id(
-/*==========*/
-	trx_id_t	trx_id)	/*!< in: trx id to search for */
-{
-	trx_t*	trx;
-
-	rw_lock_s_lock(&trx_sys->lock);
-
-	trx = trx_get_on_id_low(trx_id);
-
-	rw_lock_s_unlock(&trx_sys->lock);
-
-	return(trx);
-}
-
-/****************************************************************//**
-Returns the minumum trx id in trx list. This is the smallest id for which
+Returns the minimum trx id in trx list. This is the smallest id for which
 the trx can possibly be active. (But, you must look at the trx->state
 to find out if the minimum trx id transaction itself is active, or already
 committed.). The caller must be holding the trx_sys_t::lock in shared mode.
@@ -412,7 +394,7 @@ trx_is_active_low(
 			*corrupt = TRUE;
 		}
 	} else {
-		trx = trx_get_on_id_low(trx_id);
+		trx = trx_get_on_id(trx_id);
 
 		if (trx != NULL
 		    && trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) {

=== modified file 'storage/innobase/lock/lock0lock.c'
--- a/storage/innobase/lock/lock0lock.c	revid:marko.makela@oracle.com-20110124102828-1o7l37nibd18jwja
+++ b/storage/innobase/lock/lock0lock.c	revid:marko.makela@stripped110124125226-tecjgoda8urz5lkn
@@ -368,14 +368,13 @@ static
 ibool
 lock_rec_validate_page(
 /*===================*/
-	ibool	have_lock_trx_sys_mutex,	/*!< in: if the caller holds
-						both the lock mutex and
-						trx_sys_t::lock. */
-	ulint	space,				/*!< in: space id */
-	ulint	zip_size,			/*!< in: compressed page size
-						in bytes or 0 for uncompressed
-						pages */
-	ulint	page_no);			/*!< in: page number */
+	ibool	locked_lock_trx_sys,	/*!< in: if the caller holds
+					both the lock mutex and
+					trx_sys_t->lock. */
+	ulint	space,			/*!< in: space id */
+	ulint	zip_size,		/*!< in: compressed page size
+					in bytes or 0 for uncompressed pages */
+	ulint	page_no);		/*!< in: page number */
 #endif /* UNIV_DEBUG */
 
 /* The lock system */
@@ -587,10 +586,10 @@ lock_sys_create(
 	srv_n_lock_wait_time = 0;
 	srv_n_lock_max_wait_time = 0;
 	srv_lock_timeout_active = FALSE;
-	srv_n_lock_wait_current_count	= 0;
+	srv_n_lock_wait_current_count = 0;
 
-	lock_sys_sz = sizeof(*lock_sys) 
-		    + (OS_THREAD_MAX_N * sizeof(srv_slot_t));
+	lock_sys_sz = sizeof(*lock_sys)
+		+ OS_THREAD_MAX_N * sizeof(srv_slot_t);
 
 	lock_sys = mem_zalloc(lock_sys_sz);
 
@@ -605,8 +604,6 @@ lock_sys_create(
 
 	lock_sys->rec_hash = hash_create(n_cells);
 
-	/* hash_create_mutexes(lock_sys->rec_hash, 2, SYNC_REC_LOCK); */
-
 	lock_latest_err_file = os_file_create_tmpfile();
 	ut_a(lock_latest_err_file);
 
@@ -825,7 +822,10 @@ lock_set_lock_and_trx_wait(
 	trx_t*	trx)	/*!< in/out: trx */
 {
 	ut_ad(lock);
+	ut_ad(lock->trx == trx);
 	ut_ad(trx->lock.wait_lock == NULL);
+	ut_ad(lock_mutex_own());
+	ut_ad(trx_mutex_own(trx));
 
 	trx->lock.wait_lock = lock;
 	lock->type_mode |= LOCK_WAIT;
@@ -1721,6 +1721,7 @@ lock_rec_create(
 	const page_t*	page;
 
 	ut_ad(lock_mutex_own());
+	ut_ad(caller_owns_trx_mutex == trx_mutex_own(trx));
 
 	space = buf_block_get_space(block);
 	page_no	= buf_block_get_page_no(block);
@@ -1768,15 +1769,16 @@ lock_rec_create(
 	HASH_INSERT(lock_t, hash, lock_sys->rec_hash,
 		    lock_rec_fold(space, page_no), lock);
 
+	if (!caller_owns_trx_mutex) {
+		trx_mutex_enter(trx);
+	}
+	ut_ad(trx_mutex_own(trx));
+
 	if (UNIV_UNLIKELY(type_mode & LOCK_WAIT)) {
 
 		lock_set_lock_and_trx_wait(lock, trx);
 	}
 
-	if (!caller_owns_trx_mutex) {
-		trx_mutex_enter(trx);
-	}
-
 	UT_LIST_ADD_LAST(trx_locks, trx->lock.trx_locks, lock);
 
 	if (!caller_owns_trx_mutex) {
@@ -1921,6 +1923,7 @@ lock_rec_add_to_queue(
 	lock_t*	first_lock;
 
 	ut_ad(lock_mutex_own());
+	ut_ad(caller_owns_trx_mutex == trx_mutex_own(trx));
 #ifdef UNIV_DEBUG
 	switch (type_mode & LOCK_MODE_MASK) {
 	case LOCK_X:
@@ -2832,9 +2835,7 @@ lock_move_rec_list_end(
 				lock_rec_reset_nth_bit(lock, heap_no);
 
 				if (UNIV_UNLIKELY(type_mode & LOCK_WAIT)) {
-					trx_mutex_enter(lock->trx);
 					lock_reset_lock_and_trx_wait(lock);
-					trx_mutex_exit(lock->trx);
 				}
 
 				if (comp) {
@@ -3718,17 +3719,11 @@ lock_deadlock_recursive(
 				lock_deadlock_fputs(
 					"*** WE ROLL BACK TRANSACTION (1)\n");
 
-				wait_lock->trx->lock.was_chosen_as_deadlock_victim
-					= TRUE;
-
-				/* We need to release the transaction mutex,
-				in case  start is granted its locks once we
-				release wait_lock. */
-
 				trx_mutex_enter(wait_lock->trx);
+				wait_lock->trx->lock
+					.was_chosen_as_deadlock_victim = TRUE;
 
 				lock_cancel_waiting_and_release(wait_lock);
-
 				trx_mutex_exit(wait_lock->trx);
 
 				trx_mutex_enter(start);
@@ -4954,9 +4949,10 @@ static
 ibool
 lock_rec_queue_validate(
 /*====================*/
-	ibool			have_lock_trx_sys_mutex,
+	ibool			locked_lock_trx_sys,
 					/*!< in: if the caller holds
-					both the lock and trx sys mutexes. */
+					both the lock mutex and
+					trx_sys_t->lock. */
 	const buf_block_t*	block,	/*!< in: buffer block containing rec */
 	const rec_t*		rec,	/*!< in: record to look at */
 	dict_index_t*		index,	/*!< in: index, or NULL if not known */
@@ -4970,16 +4966,20 @@ lock_rec_queue_validate(
 	ut_a(block->frame == page_align(rec));
 	ut_ad(rec_offs_validate(rec, index, offsets));
 	ut_ad(!page_rec_is_comp(rec) == !rec_offs_comp(offsets));
+	ut_ad(lock_mutex_own() == locked_lock_trx_sys);
+#ifdef UNIV_SYNC_DEBUG
+	ut_ad(rw_lock_own(&trx_sys->lock, RW_LOCK_SHARED)
+	      == locked_lock_trx_sys);
+#endif /* UNIV_SYNC_DEBUG */
 
 	heap_no = page_rec_get_heap_no(rec);
 
-	if (!page_rec_is_user_rec(rec)) {
-
-		if (!have_lock_trx_sys_mutex) {
-			lock_mutex_enter();
+	if (!locked_lock_trx_sys) {
+		lock_mutex_enter();
+		rw_lock_s_lock(&trx_sys->lock);
+	}
 
-			rw_lock_s_lock(&trx_sys->lock);
-		}
+	if (!page_rec_is_user_rec(rec)) {
 
 		for (lock = lock_rec_get_first(block, heap_no);
 		     lock != NULL;
@@ -4996,19 +4996,7 @@ lock_rec_queue_validate(
 			}
 		}
 
-		if (!have_lock_trx_sys_mutex) {
-			rw_lock_s_unlock(&trx_sys->lock);
-
-			lock_mutex_exit();
-		}
-
-		return(TRUE);
-	}
-
-	if (!have_lock_trx_sys_mutex) {
-		lock_mutex_enter();
-
-		rw_lock_s_lock(&trx_sys->lock);
+		goto func_exit;
 	}
 
 	if (!index);
@@ -5062,11 +5050,10 @@ lock_rec_queue_validate(
 		}
 	}
 
-	if (!have_lock_trx_sys_mutex) {
-
-		rw_lock_s_unlock(&trx_sys->lock);
-
+func_exit:
+	if (!locked_lock_trx_sys) {
 		lock_mutex_exit();
+		rw_lock_s_unlock(&trx_sys->lock);
 	}
 
 	return(TRUE);
@@ -5079,14 +5066,13 @@ static
 ibool
 lock_rec_validate_page(
 /*===================*/
-	ibool	have_lock_trx_sys_mutex,	/*!< in: if the caller holds
-						both the lock and trx sys
-						mutexes. */
-	ulint	space,				/*!< in: space id */
-	ulint	zip_size,			/*!< in: compressed page size
-						in bytes or 0 for uncompressed
-						pages */
-	ulint	page_no)			/*!< in: page number */
+	ibool	locked_lock_trx_sys,	/*!< in: if the caller holds
+					both the lock mutex and
+					trx_sys_t->lock. */
+	ulint	space,			/*!< in: space id */
+	ulint	zip_size,		/*!< in: compressed page size
+					in bytes or 0 for uncompressed pages */
+	ulint	page_no)		/*!< in: page number */
 {
 	dict_index_t*	index;
 	buf_block_t*	block;
@@ -5103,10 +5089,9 @@ lock_rec_validate_page(
 	rec_offs_init(offsets_);
 
 	/* This is to preserve latching order. */
-	if (have_lock_trx_sys_mutex) {
-		rw_lock_s_unlock(&trx_sys->lock);
-
+	if (locked_lock_trx_sys) {
 		lock_mutex_exit();
+		rw_lock_s_unlock(&trx_sys->lock);
 	}
 
 	ut_ad(!lock_mutex_own());
@@ -5190,10 +5175,9 @@ function_exit:
 
 	mtr_commit(&mtr);
 
-	if (!have_lock_trx_sys_mutex) {
-		rw_lock_s_unlock(&trx_sys->lock);
-
+	if (!locked_lock_trx_sys) {
 		lock_mutex_exit();
+		rw_lock_s_unlock(&trx_sys->lock);
 	}
 
 	if (UNIV_LIKELY_NULL(heap)) {

=== modified file 'storage/innobase/row/row0vers.c'
--- a/storage/innobase/row/row0vers.c	revid:marko.makela@stripped37nibd18jwja
+++ b/storage/innobase/row/row0vers.c	revid:marko.makela@stripped
@@ -681,7 +681,7 @@ row_vers_build_for_semi_consistent_read(
 		}
 
 		rw_lock_s_lock(&trx_sys->lock);
-		version_trx = trx_get_on_id_low(version_trx_id);
+		version_trx = trx_get_on_id(version_trx_id);
 		/* version_trx->state cannot change from or to
 		NOT_STARTED while we are holding the trx_sys->lock.
 		It may change from ACTIVE to PREPARED or COMMITTED. */

=== modified file 'storage/innobase/trx/trx0trx.c'
--- a/storage/innobase/trx/trx0trx.c	revid:marko.makela@strippedja
+++ b/storage/innobase/trx/trx0trx.c	revid:marko.makela@stripped
@@ -573,7 +573,9 @@ trx_lists_init_at_db_start(void)
 			ibool	trx_created;
 
 			/* Check the trx_sys->trx_list first. */
+			rw_lock_s_lock(&trx_sys->lock);
 			trx = trx_get_on_id(undo->trx_id);
+			rw_lock_s_unlock(&trx_sys->lock);
 
 			if (trx == NULL) {
 				trx = trx_allocate_for_background();

Attachment: [text/bzr-bundle] bzr/marko.makela@oracle.com-20110124125226-tecjgoda8urz5lkn.bundle
Thread
bzr push into mysql-trunk-innodb branch (marko.makela:3453 to 3455) WL#5458marko.makela24 Jan