Skip to content

Commit

Permalink
MDEV-15326: Backport trx_t::is_referenced()
Browse files Browse the repository at this point in the history
Backport the applicable part of Sergey Vojtovich's commit
0ca2ea1 from MariaDB Server 10.3.

trx reference counter was updated under mutex and read without any
protection. This is both slow and unsafe. Use atomic operations for
reference counter accesses.
  • Loading branch information
dr-m committed Sep 4, 2019
1 parent b07beff commit dae1b3b
Show file tree
Hide file tree
Showing 8 changed files with 60 additions and 56 deletions.
2 changes: 1 addition & 1 deletion storage/innobase/include/row0vers.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ index record.
@param[in] index secondary index
@param[in] offsets rec_get_offsets(rec, index)
@return the active transaction; state must be rechecked after
trx_mutex_enter(), and trx_release_reference() must be invoked
trx_mutex_enter(), and trx->release_reference() must be invoked
@retval NULL if the record was committed */
trx_t*
row_vers_impl_x_locked(
Expand Down
8 changes: 6 additions & 2 deletions storage/innobase/include/trx0sys.ic
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,6 @@ inline trx_t* trx_rw_is_active(trx_id_t trx_id, bool* corrupt, bool ref_count)
mutex_enter(trx_mutex);
ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED));
ut_ad(trx->id == trx_id);
ut_ad(trx->n_ref >= 0);
if (trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)) {
/* We have an early state check here to avoid
committer starvation in a wait loop for
Expand All @@ -357,7 +356,12 @@ inline trx_t* trx_rw_is_active(trx_id_t trx_id, bool* corrupt, bool ref_count)
rechecked by the caller after reacquiring the mutex. */
trx = NULL;
} else {
++trx->n_ref;
/* The reference could be safely incremented after
releasing one of trx_mutex or trx_sys->mutex.
Holding trx->mutex here may prevent a few false
references that could have a negative performance
impact on trx_commit_in_memory(). */
trx->reference();
}
mutex_exit(trx_mutex);
}
Expand Down
62 changes: 41 additions & 21 deletions storage/innobase/include/trx0trx.h
Original file line number Diff line number Diff line change
Expand Up @@ -500,18 +500,6 @@ void
trx_set_rw_mode(
trx_t* trx);

/**
Release the transaction. Decrease the reference count.
@param trx Transaction that is being released */
UNIV_INLINE
void
trx_release_reference(
trx_t* trx);

/**
Check if the transaction is being referenced. */
#define trx_is_referenced(t) ((t)->n_ref > 0)

/**
Transactions that aren't started by the MySQL server don't set
the trx_t::mysql_thd field. For such transactions we set the lock
Expand Down Expand Up @@ -571,7 +559,7 @@ Check transaction state */
ut_ad(trx_state_eq((t), TRX_STATE_NOT_STARTED)); \
ut_ad(!(t)->id); \
ut_ad(!(t)->has_logged()); \
ut_ad(!(t)->n_ref); \
ut_ad(!(t)->is_referenced()); \
ut_ad(!MVCC::is_view_active((t)->read_view)); \
ut_ad((t)->lock.wait_thr == NULL); \
ut_ad(UT_LIST_GET_LEN((t)->lock.trx_locks) == 0); \
Expand Down Expand Up @@ -801,6 +789,19 @@ struct trx_rsegs_t {
};

struct trx_t {
private:
/**
Count of references.
We can't release the locks nor commit the transaction until this reference
is 0. We can change the state to TRX_STATE_COMMITTED_IN_MEMORY to signify
that it is no longer "active".
*/

int32_t n_ref;


public:
TrxMutex mutex; /*!< Mutex protecting the fields
state and lock (except some fields
of lock, which are protected by
Expand Down Expand Up @@ -1100,14 +1101,6 @@ struct trx_t {
const char* start_file; /*!< Filename where it was started */
#endif /* UNIV_DEBUG */

lint n_ref; /*!< Count of references, protected
by trx_t::mutex. We can't release the
locks nor commit the transaction until
this reference is 0. We can change
the state to COMMITTED_IN_MEMORY to
signify that it is no longer
"active". */

XID* xid; /*!< X/Open XA transaction
identification to identify a
transaction branch */
Expand Down Expand Up @@ -1184,6 +1177,33 @@ struct trx_t {
/** Release any explicit locks of a committing transaction. */
inline void release_locks();


bool is_referenced()
{
return my_atomic_load32_explicit(&n_ref, MY_MEMORY_ORDER_RELAXED) > 0;
}


void reference()
{
#ifdef UNIV_DEBUG
int32_t old_n_ref=
#endif
my_atomic_add32_explicit(&n_ref, 1, MY_MEMORY_ORDER_RELAXED);
ut_ad(old_n_ref >= 0);
}


void release_reference()
{
#ifdef UNIV_DEBUG
int32_t old_n_ref=
#endif
my_atomic_add32_explicit(&n_ref, -1, MY_MEMORY_ORDER_RELAXED);
ut_ad(old_n_ref > 0);
}


private:
/** Assign a rollback segment for modifying temporary tables.
@return the assigned rollback segment */
Expand Down
17 changes: 0 additions & 17 deletions storage/innobase/include/trx0trx.ic
Original file line number Diff line number Diff line change
Expand Up @@ -210,23 +210,6 @@ ok:
trx->dict_operation = op;
}

/**
Release the transaction. Decrease the reference count.
@param trx Transaction that is being released */
UNIV_INLINE
void
trx_release_reference(
trx_t* trx)
{
trx_mutex_enter(trx);

ut_ad(trx->n_ref > 0);
--trx->n_ref;

trx_mutex_exit(trx);
}


/**
@param trx Get the active view for this transaction, if one exists
@return the transaction's read view or NULL if one not assigned. */
Expand Down
9 changes: 5 additions & 4 deletions storage/innobase/lock/lock0lock.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5772,10 +5772,12 @@ lock_rec_convert_impl_to_expl_for_trx(
trx_t* trx, /*!< in/out: active transaction */
ulint heap_no)/*!< in: rec heap number to lock */
{
DEBUG_SYNC_C("before_lock_rec_convert_impl_to_expl_for_trx");
ut_ad(page_rec_is_leaf(rec));

DEBUG_SYNC_C("before_lock_rec_convert_impl_to_expl_for_trx");
lock_mutex_enter();
trx_mutex_enter(trx);
ut_ad(trx->is_referenced());
ut_ad(!trx_state_eq(trx, TRX_STATE_NOT_STARTED));

if (!trx_state_eq(trx, TRX_STATE_COMMITTED_IN_MEMORY)
Expand All @@ -5786,9 +5788,8 @@ lock_rec_convert_impl_to_expl_for_trx(
}

lock_mutex_exit();
ut_ad(trx->n_ref > 0);
--trx->n_ref;
trx_mutex_exit(trx);
trx->release_reference();

DEBUG_SYNC_C("after_lock_rec_convert_impl_to_expl_for_trx");
}
Expand Down Expand Up @@ -5830,7 +5831,7 @@ lock_rec_convert_impl_to_expl(
if (trx != 0) {
ulint heap_no = page_rec_get_heap_no(rec);

ut_ad(trx_is_referenced(trx));
ut_ad(trx->is_referenced());

/* If the transaction is still active and has no
explicit x-lock set on the record, set one for it.
Expand Down
2 changes: 1 addition & 1 deletion storage/innobase/row/row0sel.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5087,7 +5087,7 @@ row_search_mvcc(
rec, index, offsets)) {
/* The record belongs to an active
transaction. We must acquire a lock. */
trx_release_reference(trx);
trx->release_reference();
} else {
/* The secondary index record does not
point to a delete-marked clustered index
Expand Down
8 changes: 4 additions & 4 deletions storage/innobase/row/row0vers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ index record.
@param[in] offsets rec_get_offsets(rec, index)
@param[in,out] mtr mini-transaction
@return the active transaction; state must be rechecked after
trx_mutex_enter(), and trx_release_reference() must be invoked
trx_mutex_enter(), and trx->release_reference() must be invoked
@retval NULL if the record was committed */
UNIV_INLINE
trx_t*
Expand Down Expand Up @@ -213,7 +213,7 @@ row_vers_impl_x_locked_low(
created with a clear delete-mark flag.
(We never insert a delete-marked record.) */
not_locked:
trx_release_reference(trx);
trx->release_reference();
trx = 0;
}

Expand Down Expand Up @@ -362,7 +362,7 @@ index record.
@param[in] index secondary index
@param[in] offsets rec_get_offsets(rec, index)
@return the active transaction; state must be rechecked after
trx_mutex_enter(), and trx_release_reference() must be invoked
trx_mutex_enter(), and trx->release_reference() must be invoked
@retval NULL if the record was committed */
trx_t*
row_vers_impl_x_locked(
Expand Down Expand Up @@ -408,7 +408,7 @@ row_vers_impl_x_locked(
trx = row_vers_impl_x_locked_low(
clust_rec, clust_index, rec, index, offsets, &mtr);

ut_ad(trx == 0 || trx_is_referenced(trx));
ut_ad(trx == 0 || trx->is_referenced());
}

mtr_commit(&mtr);
Expand Down
8 changes: 2 additions & 6 deletions storage/innobase/trx/trx0trx.cc
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ inline void trx_t::commit_state()
background thread. To avoid this race we unconditionally unset
the is_recovered flag. */
is_recovered= false;
ut_ad(id || !trx_is_referenced(this));
ut_ad(id || !is_referenced());
}

/** Release any explicit locks of a committing transaction. */
Expand Down Expand Up @@ -1705,13 +1705,9 @@ trx_commit_in_memory(
/* Wait for any implicit-to-explicit lock
conversions to cease, so that there will be no
race condition in lock_release(). */
trx_mutex_enter(trx);
while (UNIV_UNLIKELY(trx_is_referenced(trx))) {
trx_mutex_exit(trx);
while (UNIV_UNLIKELY(trx->is_referenced())) {
ut_delay(srv_spin_wait_delay);
trx_mutex_enter(trx);
}
trx_mutex_exit(trx);

trx->release_locks();
trx->id = 0;
Expand Down

0 comments on commit dae1b3b

Please sign in to comment.