Get rid of trx->read_view pointer juggling

trx->read_view|= 1 was done in a silly attempt to fix race condition
where trx->read_view was closed without trx_sys.mutex lock by read-only
trasnactions.

This just made the problem less likely to happen. In fact there was race
condition in const version of trx_get_read_view(): pointer may change to
garbage any moment after MVCC::is_view_active(trx->read_view) check and
before this function returns.

This patch doesn't fix this race condition, but rather makes it's
consequences less destructive.

storage/innobase/handler/ha_innodb.cc

@@ -16222,7 +16222,7 @@ ha_innobase::external_lock(
 		} else if (trx->isolation_level <= TRX_ISO_READ_COMMITTED
 			   && MVCC::is_view_active(trx->read_view)) {
 			mutex_enter(&trx_sys.mutex);
-			trx_sys.mvcc.view_close(trx->read_view, true);
+			trx_sys.mvcc.view_close(trx->read_view);
 			mutex_exit(&trx_sys.mutex);
 		}
 	}
@@ -16890,7 +16890,7 @@ ha_innobase::store_lock(
 			/* At low transaction isolation levels we let
 			each consistent read set its own snapshot */
 			mutex_enter(&trx_sys.mutex);
-			trx_sys.mvcc.view_close(trx->read_view, true);
+			trx_sys.mvcc.view_close(trx->read_view);
 			mutex_enter(&trx_sys.mutex);
 		}
 	}

storage/innobase/include/read0read.h

@@ -50,9 +50,8 @@ class MVCC {
 
 	/**
 	Close a view created by the above function.
-	@para view		view allocated by trx_open.
-	@param own_mutex	true if caller owns trx_sys_t::mutex */
-	void view_close(ReadView*& view, bool own_mutex);
+	@param view		view allocated by trx_open. */
+	void view_close(ReadView*& view);
 
 	/** Clones the oldest view and stores it in view. No need to
 	call view_close(). The caller owns the view that is passed in.
@@ -69,9 +68,7 @@ class MVCC {
 	@return true if the view is active and valid */
 	static bool is_view_active(ReadView* view)
 	{
-		ut_a(view != reinterpret_cast<ReadView*>(0x1));
-
-		return(view != NULL && !(intptr_t(view) & 0x1));
+		return view && !view->is_closed();
 	}
 
 private:

storage/innobase/include/read0types.h

@@ -207,6 +207,11 @@ class ReadView {
 		return(m_closed);
 	}
 
+	void set_closed(bool closed)
+	{
+		m_closed= closed;
+	}
+
 	/**
 	Write the limits to the file.
 	@param file		file to write to */

storage/innobase/include/trx0trx.h

@@ -307,14 +307,6 @@ trx_get_read_view(
 /*==============*/
 	trx_t*	trx);
 
-/****************************************************************//**
-@return the transaction's read view or NULL if one not assigned. */
-UNIV_INLINE
-const ReadView*
-trx_get_read_view(
-/*==============*/
-	const trx_t*	trx);
-
 /****************************************************************//**
 Prepares a transaction for commit/rollback. */
 void

storage/innobase/include/trx0trx.ic

@@ -222,17 +222,6 @@ trx_get_read_view(
 	return(!MVCC::is_view_active(trx->read_view) ? NULL : trx->read_view);
 }
 
-/**
-@param trx		Get the active view for this transaction, if one exists
-@return the transaction's read view or NULL if one not assigned. */
-UNIV_INLINE
-const ReadView*
-trx_get_read_view(
-	const trx_t*	trx)
-{
-	return(!MVCC::is_view_active(trx->read_view) ? NULL : trx->read_view);
-}
-
 /**
 @param[in] trx		Transaction to check
 @return true if the transaction is a high priority transaction.*/

storage/innobase/lock/lock0lock.cc

@@ -5719,9 +5719,14 @@ lock_trx_print_wait_and_mvcc_state(
 
 	trx_print_latched(file, trx, 600);
 
-	const ReadView*	read_view = trx_get_read_view(trx);
-
-	if (read_view != NULL) {
+	/* Note: this read_view access is data race. Further
+	read_view->is_active() check is race condition. But it should
+	"kind of work" because read_view is freed only at shutdown.
+	Worst thing that may happen is that it'll get transferred to
+	another thread and print wrong values. */
+	const ReadView*	read_view = trx->read_view;
+
+	if (read_view != NULL && !read_view->is_closed()) {
 		read_view->print_limits(file);
 	}
 

storage/innobase/read/read0read.cc

@@ -665,36 +665,15 @@ MVCC::size() const
 
 /**
 Close a view created by the above function.
-@para view		view allocated by trx_open.
-@param own_mutex	true if caller owns trx_sys_t::mutex */
+@param view		view allocated by trx_open. */
 
 void
-MVCC::view_close(ReadView*& view, bool own_mutex)
+MVCC::view_close(ReadView*& view)
 {
-	uintptr_t	p = reinterpret_cast<uintptr_t>(view);
-
-	/* Note: The assumption here is that AC-NL-RO transactions will
-	call this function with own_mutex == false. */
-	if (!own_mutex) {
-		/* Sanitise the pointer first. */
-		ReadView*	ptr = reinterpret_cast<ReadView*>(p & ~1);
-
-		/* Note this can be called for a read view that
-		was already closed. */
-		ptr->m_closed = true;
-
-		/* Set the view as closed. */
-		view = reinterpret_cast<ReadView*>(p | 0x1);
-	} else {
-		view = reinterpret_cast<ReadView*>(p & ~1);
-
-		view->close();
-
-		UT_LIST_REMOVE(m_views, view);
-		UT_LIST_ADD_LAST(m_free, view);
-
-		ut_ad(validate());
-
-		view = NULL;
-	}
+	ut_ad(mutex_own(&trx_sys.mutex));
+	view->close();
+	UT_LIST_REMOVE(m_views, view);
+	UT_LIST_ADD_LAST(m_free, view);
+	ut_ad(validate());
+	view = NULL;
 }

storage/innobase/trx/trx0trx.cc

@@ -678,7 +678,7 @@ trx_disconnect_from_mysql(
 	UT_LIST_REMOVE(trx_sys.mysql_trx_list, trx);
 
 	if (trx->read_view != NULL) {
-		trx_sys.mvcc.view_close(trx->read_view, true);
+		trx_sys.mvcc.view_close(trx->read_view);
 	}
 
 	if (prepared) {
@@ -1539,7 +1539,7 @@ trx_erase_lists(
 	if (trx->rsegs.m_redo.rseg && trx->read_view) {
 		ut_ad(!trx->read_only);
 		mutex_enter(&trx_sys.mutex);
-		trx_sys.mvcc.view_close(trx->read_view, true);
+		trx_sys.mvcc.view_close(trx->read_view);
 	} else {
 
 		mutex_enter(&trx_sys.mutex);
@@ -1598,7 +1598,7 @@ trx_commit_in_memory(
 		ut_ad(trx_state_eq(trx, TRX_STATE_ACTIVE));
 
 		if (trx->read_view != NULL) {
-			trx_sys.mvcc.view_close(trx->read_view, false);
+			trx->read_view->set_closed(true);
 		}
 
 		MONITOR_INC(MONITOR_TRX_NL_RO_COMMIT);
@@ -1630,8 +1630,7 @@ trx_commit_in_memory(
 		if (trx->read_only || trx->rsegs.m_redo.rseg == NULL) {
 			MONITOR_INC(MONITOR_TRX_RO_COMMIT);
 			if (trx->read_view != NULL) {
-				trx_sys.mvcc.view_close(
-					trx->read_view, false);
+				trx->read_view->set_closed(true);
 			}
 		} else {
 			trx_update_mod_tables_timestamp(trx);