bzr push into mysql-trunk-innodb branch (marko.makela:3442 to 3443) WL#5458

[marko.makela]

 3443 Marko Mäkelä	2011-01-20
      WL#5458 clean-up: Clarify the latching rules for trx->state changes.
      
      This rules is already documented in trx0trx.h and possibly some
      other places in the code:
      
      When a transaction is (or may be) in the trx->mysql_trx_list or
      trx_sys->trx_list, changing its state to or from TRX_STATE_NOT_STARTED
      is only allowed while holding trx_sys->lock in exclusive mode.
      
      trx_start_low(): Change the state from TRX_STATE_NOT_STARTED to
      TRX_STATE_ACTIVE while holding trx_sys->lock.
      
      trx_commit(): Change the state to TRX_STATE_NOT_STARTED while holding
      trx_sys->lock. Remove the transaction from trx_sys->trx_list before
      removing the read view and flushing the redo log.
      
      trx_cleanup_at_db_startup(): Change the state to TRX_STATE_NOT_STARTED
      while holding trx_sys->lock.
      
      lock_print_info_all_transactions(), read_view_open_now_low(): Update
      comments about reads of trx->state that are no longer dirty.
      
      rb://573 approved by Sunny Bains

    modified:
      storage/innobase/lock/lock0lock.c
      storage/innobase/read/read0read.c
      storage/innobase/trx/trx0trx.c
 3442 Vasil Dimov	2011-01-19
      Rename the C macros for MySQL version to be consistent with CMake variables
      
      Also set InnoDB version automatically from MySQL version so I do not have
      to adjust it every time after a release. If the versions ever go out of
      sync, then the InnoDB version can be adjusted in univ.i.

    modified:
      config.h.cmake
      storage/innobase/include/univ.i
=== modified file 'storage/innobase/lock/lock0lock.c'
--- a/storage/innobase/lock/lock0lock.c	revid:vasil.dimov@stripped0119140439-0or7eg74usmrnpcb
+++ b/storage/innobase/lock/lock0lock.c	revid:marko.makela@strippedf3nm75i1guftugt
@@ -4742,9 +4742,10 @@ lock_print_info_all_transactions(
 	     trx = UT_LIST_GET_NEXT(mysql_trx_list, trx)) {
 
 		ut_ad(trx->in_mysql_trx_list);
-		/* Note we are doing a dirty read here and it is possible
-		for the transaction state to change while we are printing
-		the transaction. This should be OK. */
+		/* trx->state cannot change from or to NOT_STARTED
+		while we are holding the trx_sys->lock. It may change
+		from ACTIVE to PREPARED, but it may not change to
+		COMMITTED, because we are holding the lock_sys->mutex. */
 		if (trx->state == TRX_STATE_NOT_STARTED) {
 			fputs("---", file);
 			trx_print_latched(file, trx, 600);

=== modified file 'storage/innobase/read/read0read.c'
--- a/storage/innobase/read/read0read.c	revid:vasil.dimov@stripped9-0or7eg74usmrnpcb
+++ b/storage/innobase/read/read0read.c	revid:marko.makela@strippeduftugt
@@ -374,9 +374,9 @@ read_view_open_now_low(
 	     trx = UT_LIST_GET_NEXT(trx_list, trx)) {
 
 		ut_ad(trx->in_trx_list);
-		/* Note: We are doing a dirty read of the trx_t::state
-		without the cover of the trx_t::mutex. The state change
-		to TRX_STATE_PREPARED is done using only the trx_t::mutex. */
+		/* 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. */
 
 		if (trx->id != cr_trx_id
 		    && (trx->state == TRX_STATE_ACTIVE

=== modified file 'storage/innobase/trx/trx0trx.c'
--- a/storage/innobase/trx/trx0trx.c	revid:vasil.dimov@strippedpcb
+++ b/storage/innobase/trx/trx0trx.c	revid:marko.makela@stripped
@@ -650,13 +650,17 @@ trx_start_low(
 
 	trx->start_time = ut_time();
 
-	trx->state = TRX_STATE_ACTIVE;
-
 	ut_a(trx->rseg == NULL);
 	trx->rseg = rseg;
 
 	rw_lock_x_lock(&trx_sys->lock);
 
+	/* If this transaction came from trx_allocate_for_mysql(),
+	trx->in_mysql_trx_list would hold. In that case, the trx->state
+	change must be protected by the trx_sys->lock, so that
+	lock_print_info_all_transactions() will have a consistent view. */
+	trx->state = TRX_STATE_ACTIVE;
+
 	trx->id = trx_sys_get_new_trx_id();
 
 	UT_LIST_ADD_FIRST(trx_list, trx_sys->trx_list, trx);
@@ -766,6 +770,20 @@ trx_commit(
 
 	lock_trx_release_locks(trx);
 
+	/* Remove the transaction from the list of active transactions
+	now that it no longer holds any user locks. */
+	rw_lock_x_lock(&trx_sys->lock);
+	ut_ad(trx->in_trx_list);
+	ut_d(trx->in_trx_list = FALSE);
+	UT_LIST_REMOVE(trx_list, trx_sys->trx_list, trx);
+	/* If this transaction came from trx_allocate_for_mysql(),
+	trx->in_mysql_trx_list would hold. In that case, the
+	trx->state change must be protected by trx_sys->lock, so that
+	lock_print_info_all_transactions() will have a consistent view. */
+	trx->state = TRX_STATE_NOT_STARTED;
+	ut_ad(trx_sys_validate_trx_list());
+	rw_lock_x_unlock(&trx_sys->lock);
+
 	if (trx->global_read_view != NULL) {
 		read_view_remove(trx->global_read_view);
 
@@ -849,25 +867,6 @@ trx_commit(
 
 	ut_ad(trx->lock.wait_thr == NULL);
 	ut_ad(UT_LIST_GET_LEN(trx->lock.trx_locks) == 0);
-
-	trx->state = TRX_STATE_NOT_STARTED;
-
-	rw_lock_x_lock(&trx_sys->lock);
-
-	/* FIXME: We should remove the transaction from the active
-	list earlier rather than here. This should simplify rules
-	about the transaction's state. One we enter this function
-	there is no going back and I don't see why need to wait
-	till the end to remove it from the sys list. */
-
-	ut_ad(trx->in_trx_list);
-	ut_d(trx->in_trx_list = FALSE);
-	UT_LIST_REMOVE(trx_list, trx_sys->trx_list, trx);
-
-	ut_ad(trx_sys_validate_trx_list());
-
-	rw_lock_x_unlock(&trx_sys->lock);
-
 	ut_ad(!trx->in_trx_list);
 	/* trx->in_mysql_trx_list would hold between
 	trx_allocate_for_mysql() and trx_free_for_mysql(). It does not
@@ -895,13 +894,19 @@ trx_cleanup_at_db_startup(
 
 	rw_lock_x_lock(&trx_sys->lock);
 
-	trx->state = TRX_STATE_NOT_STARTED;
-
 	UT_LIST_REMOVE(trx_list, trx_sys->trx_list, trx);
 	ut_ad(trx->in_trx_list);
 	ut_d(trx->in_trx_list = FALSE);
 
 	rw_lock_x_unlock(&trx_sys->lock);
+
+	/* Change the transaction state without mutex protection, now
+	that it no longer is in the trx_list. Recovered transactions
+	are never placed in the mysql_trx_list. */
+	ut_ad(trx->is_recovered);
+	ut_ad(!trx->in_trx_list);
+	ut_ad(!trx->in_mysql_trx_list);
+	trx->state = TRX_STATE_NOT_STARTED;
 }
 
 /********************************************************************//**

Attachment: [text/bzr-bundle] bzr/[e-mail]