MDEV-15326 after-merge fixes

trx_t::is_recovered: Revert most of the changes that were made by the
merge of MDEV-15326 from 10.2. The trx_sys.rw_trx_hash and the recovery
of transactions at startup is quite different in 10.3.

trx_free_at_shutdown(): Avoid excessive mutex protection. Reading fields
that can only be modified by the current thread (owning the transaction)
can be done outside mutex.

trx_t::commit_state(): Restore a tighter assertion.

trx_rollback_recovered(): Clarify why there is no potential race condition
with other transactions.

lock_trx_release_locks(): Merge with trx_t::release_locks(),
and avoid holding lock_sys.mutex unnecessarily long.

rw_trx_hash_t::find(): Remove redundant code, and avoid starving the
committer by checking trx_t::state before trx_t::reference().

3 files changed, 36 insertions, 30 deletions

diff --git a/storage/innobase/trx/trx0purge.cc b/storage/innobase/trx/trx0purge.cc
index b6206c9b3be..475babb6195 100644
--- a/storage/innobase/trx/trx0purge.cc
+++ b/storage/innobase/trx/trx0purge.cc
@@ -252,7 +252,7 @@ trx_purge_add_undo_to_history(const trx_t* trx, trx_undo_t*& undo, mtr_t* mtr)
 	/* After the purge thread has been given permission to exit,
 	we may roll back transactions (trx->undo_no==0)
 	in THD::cleanup() invoked from unlink_thd() in fast shutdown,
-	or in trx_rollback_resurrected() in slow shutdown.
+	or in trx_rollback_recovered() in slow shutdown.
 
 	Before any transaction-generating background threads or the
 	purge have been started, recv_recovery_rollback_active() can
diff --git a/storage/innobase/trx/trx0roll.cc b/storage/innobase/trx/trx0roll.cc
index 709ec98259c..a52724444ec 100644
--- a/storage/innobase/trx/trx0roll.cc
+++ b/storage/innobase/trx/trx0roll.cc
@@ -764,12 +764,8 @@ static my_bool trx_rollback_recovered_callback(rw_trx_hash_element_t *element,
   mutex_enter(&element->mutex);
   if (trx_t *trx= element->trx)
   {
-    /* The trx->is_recovered flag and trx->state are set
-    atomically under the protection of the trx->mutex in
-    trx_t::commit_state(). We do not want to accidentally clean up
-    a non-recovered transaction here. */
     mutex_enter(&trx->mutex);
-    if (trx->is_recovered && trx_state_eq(trx, TRX_STATE_ACTIVE))
+    if (trx_state_eq(trx, TRX_STATE_ACTIVE) && trx->is_recovered)
       trx_list->push_back(trx);
     mutex_exit(&trx->mutex);
   }
@@ -815,7 +811,8 @@ void trx_rollback_recovered(bool all)
 
     ut_ad(trx);
     ut_d(trx_mutex_enter(trx));
-    ut_ad(trx->is_recovered && trx_state_eq(trx, TRX_STATE_ACTIVE));
+    ut_ad(trx->is_recovered);
+    ut_ad(trx_state_eq(trx, TRX_STATE_ACTIVE));
     ut_d(trx_mutex_exit(trx));
 
     if (!srv_is_being_started && !srv_undo_sources && srv_fast_shutdown)
@@ -831,6 +828,22 @@ void trx_rollback_recovered(bool all)
         ut_ad(!srv_undo_sources);
         ut_ad(srv_fast_shutdown);
 discard:
+        /* Note: before kill_server() invoked innobase_end() via
+        unireg_end(), it invoked close_connections(), which should initiate
+        the rollback of any user transactions via THD::cleanup() in the
+        connection threads, and wait for all THD::cleanup() to complete.
+        So, no active user transactions should exist at this point.
+
+        srv_undo_sources=false was cleared early in innobase_end().
+
+        Generally, the server guarantees that all connections using
+        InnoDB must be disconnected by the time we are reaching this code,
+        be it during shutdown or UNINSTALL PLUGIN.
+
+        Because there is no possible race condition with any
+        concurrent user transaction, we do not have to invoke
+        trx->commit_state() or wait for !trx->is_referenced()
+        before trx_sys.deregister_rw(trx). */
         trx_sys.deregister_rw(trx);
         trx_free_at_shutdown(trx);
       }
diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc
index 6cbf0c273d9..f612be4d177 100644
--- a/storage/innobase/trx/trx0trx.cc
+++ b/storage/innobase/trx/trx0trx.cc
@@ -463,6 +463,9 @@ void trx_free(trx_t*& trx)
 /** Transition to committed state, to release implicit locks. */
 inline void trx_t::commit_state()
 {
+  ut_ad(state == TRX_STATE_PREPARED
+	|| state == TRX_STATE_PREPARED_RECOVERED
+	|| state == TRX_STATE_ACTIVE);
   /* This makes the transaction committed in memory and makes its
   changes to data visible to other transactions. NOTE that there is a
   small discrepancy from the strict formal visibility rules here: a
@@ -475,23 +478,9 @@ inline void trx_t::commit_state()
   makes modifications to the database, will get an lsn larger than the
   committing transaction T. In the case where the log flush fails, and
   T never gets committed, also T2 will never get committed. */
-  ut_ad(trx_mutex_own(this));
-  ut_ad(state != TRX_STATE_NOT_STARTED);
-  ut_ad(state != TRX_STATE_COMMITTED_IN_MEMORY
-	|| (is_recovered && !UT_LIST_GET_LEN(lock.trx_locks)));
+  trx_mutex_enter(this);
   state= TRX_STATE_COMMITTED_IN_MEMORY;
-
-  /* If the background thread trx_rollback_or_clean_recovered()
-  is still active then there is a chance that the rollback
-  thread may see this trx as COMMITTED_IN_MEMORY and goes ahead
-  to clean it up calling trx_cleanup_at_db_startup(). This can
-  happen in the case we are committing a trx here that is left
-  in PREPARED state during the crash. Note that commit of the
-  rollback of a PREPARED trx happens in the recovery thread
-  while the rollback of other transactions happen in the
-  background thread. To avoid this race we unconditionally unset
-  the is_recovered flag. */
-  is_recovered= false;
+  trx_mutex_exit(this);
   ut_ad(id || !is_referenced());
 }
 
@@ -499,18 +488,24 @@ inline void trx_t::commit_state()
 inline void trx_t::release_locks()
 {
   DBUG_ASSERT(state == TRX_STATE_COMMITTED_IN_MEMORY);
+  DBUG_ASSERT(!is_referenced());
 
   if (UT_LIST_GET_LEN(lock.trx_locks))
-    lock_trx_release_locks(this);
-  else
-    lock.table_locks.clear(); // Work around a bug
+  {
+    lock_release(this);
+    lock.n_rec_locks = 0;
+    ut_ad(UT_LIST_GET_LEN(lock.trx_locks) == 0);
+    ut_ad(ib_vector_is_empty(autoinc_locks));
+    mem_heap_empty(lock.lock_heap);
+  }
+
+  lock.table_locks.clear(); /* outside "if" to work around MDEV-20483 */
 }
 
 /** At shutdown, frees a transaction object. */
 void
 trx_free_at_shutdown(trx_t *trx)
 {
-	trx_mutex_enter(trx);
 	ut_ad(trx->is_recovered);
 	ut_a(trx_state_eq(trx, TRX_STATE_PREPARED)
 	     || trx_state_eq(trx, TRX_STATE_PREPARED_RECOVERED)
@@ -525,7 +520,6 @@ trx_free_at_shutdown(trx_t *trx)
 	ut_a(trx->magic_n == TRX_MAGIC_N);
 
 	trx->commit_state();
-	trx_mutex_exit(trx);
 	trx->release_locks();
 	trx_undo_free_at_shutdown(trx);
 
@@ -1369,9 +1363,7 @@ trx_commit_in_memory(
 		DBUG_LOG("trx", "Autocommit in memory: " << trx);
 		trx->state = TRX_STATE_NOT_STARTED;
 	} else {
-		trx_mutex_enter(trx);
 		trx->commit_state();
-		trx_mutex_exit(trx);
 
 		if (trx->id) {
 			trx_sys.deregister_rw(trx);
@@ -1397,6 +1389,7 @@ trx_commit_in_memory(
 		} else {
 			trx_update_mod_tables_timestamp(trx);
 			MONITOR_INC(MONITOR_TRX_RW_COMMIT);
+			trx->is_recovered = false;
 		}
 	}