MDEV-36684 - main.mdl_sync fails under valgrind (test for Bug#42643)

[svoj]

Valgrind is single threaded and only changes threads as part of system calls or waits.

I found some busy loops where the server assumes that some other thread will change the state, which will not happen with valgrind.

Added VALGRIND_YIELD to the loops, which calls pthread_yield() if HAVE_VALGRIND is defined.

Added pthread_yield() to the loops in table_cache.

We should consider changing some of the VALGRIND_YIELD calls to call pthread_yield() as busy loop without any sleep() is usually a bad thing.

Reviewer: [email protected]

[CLAassistant]

All committers have signed the CLA.

[dr-m]

I’d use std::this_thread::yield() in C++ code. But I don’t think it will make much difference, other than removing a dependency on POSIX or our POSIX simulation layer (#define pthread_yield()).

[svoj]

I’d use std::this_thread::yield() in C++ code. But I don’t think it will make much difference, other than removing a dependency on POSIX or our POSIX simulation layer (#define pthread_yield()).

Yes, I also suggested std::this_thread::yield() when reviewing it, same thinking.

[svoj]

@dr-m retargeted for 10.6, moved to std::this_thread::yield(). Please merge if you agree.

[dr-m]

I think that we need a better solution for purge_sys_t::close_and_reopen() where the thread would yield while not holding any metadata locks or table handles. My other comments are suggestions of follow-up improvements, not directly related to improving the Valgrind experience.

[dr-m]

This is duplicating the section of code 63 lines earlier. In fact, a wider section of the code block starting with if (unlikely(error)) is being duplicated. I think that a follow-up fix would be useful where we’d do the following:

  if (unlikely(error))
  {
    if (!(error= open_tables_handle_error(error, …)))
      goto restart;
    goto error;
  }

and there would be a new function ATTRIBUTE_COLD static bool open_tables_error_handler(bool, …) that would include the duplicated code, including the call to std::this_thread::yield().

[svoj]

Right. There was a change in 10.11 though, that makes it different by one line at least. I wouldn't dare doing it in GA versions though.

[dr-m]

Would the following be a correct optimization?

  mysql_mutex_lock(&element->LOCK_table_share);
  lf_hash_search_unpin(thd->tdc_hash_pins);
  if (!(share= element->share))
  {
    mysql_mutex_unlock(&element->LOCK_table_share);
    std::this_thread::yield();
    goto retry;
  }

[svoj]

Well, depending on what you're trying to achieve. LOCK_table_share is a per-share mutex, it is not locked for table DML, but still can become contested for views. The less we have under its protection the better. So, this will make things slightly worse in this regard, and that was the main reason to keep it out of LOCK_table_share.

Speaking of correctness: lf_hash_search_unpin() marks element unused. In current LF_HASH implementation it means element is available for reuse. Taking it into account as well as considering how table definition cache is using it, nothing bad is going to happen. But I'd say it is like relying on undefined behaviour. Once it is unpinned it becomes property of LF_HASH. If it happens before element->share check, in theory we can expect LF_HASH to free/corrupt its memory. So it is not good from correctness point of view either.

What we get if we do lf_hash_search_unpin() earlier? It means we mark element for reuse earlier, that is another thread may reuse it rather than allocating it anew.

[dr-m]

What we would gain is avoiding some code duplication and instruction cache bloat. Another variant would avoid the potential race condition:

  mysql_mutex_lock(&element->LOCK_table_share);
  share= element->share;
  lf_hash_search_unpin(thd->tdc_hash_pins);
  if (!share)
  {
    mysql_mutex_unlock(&element->LOCK_table_share);
    std::this_thread::yield();
    goto retry;
  }

[svoj]

It won't work either: mysql_mutex_unlock(&element->LOCK_table_share) would access potentially corrupt memory.

I can foresee your question "Why it is safe to unpin right after this block?" Presence of element->share guarantees there is no concurrent delete ongoing. Or if there is, it is waiting on element->LOCK_table_share mutex and thus it is safe to unpin it immediately after this block.

I understand your concern, I'd have implemented it as you suggested originally (back 10+ years ago), but I didn't see the way to arrange code better. Nor do I see it now.

[dr-m]

If we are going to insert this yield unconditionally (outside Valgrind builds), I think that the correct place for it would be right after closing all tables:

  m_active= false;
  wait_FTS(false);
  m_active= true;

In fact, maybe the following should be rewritten to be Valgrind friendly:

void purge_sys_t::wait_FTS(bool also_sys)
{
  for (const uint32_t mask= also_sys ? ~0U : ~PAUSED_SYS; m_FTS_paused & mask;)
    std::this_thread::sleep_for(std::chrono::milliseconds(10));
}

Maybe we should just add a call to std::this_thread::yield() at the beginning of this function? Because purge_sys.m_FTS_paused is being modified by atomic operations, I think that we need an actual sleep in the loop; invoking only a yield would likely result in excessive CPU consumption.

[svoj]

That's a good point. Under complex locking scenarios, like:

purge: locks t2
con1: holds MDL_EXCLUSIVE on t1, waiting for MDL_EXCLUSIVE on t2
purge: locks t1, deadlocks (t1 lock not acquired), yields
con1: can't proceed as t2 is still locked
purge: locks t1, deadlocks (t1 lock not acquired), yields
...

In this case yielding before trx_purge_close_tables() won't fix the problem we're trying to solve.

I don't feel like wait_FTS() is the right name for this method, but I agree that yield should be moved there. Will get it fixed.

[svoj]

@dr-m PR updated. I can see Monty pushed his aae9b50 to main, so you'd have to cleanup VALGRIND_YIELD once this one is merged up to main.

[dr-m]

This will need to be rebased once more, because a failing builder was made mandatory for branch protection.

[svoj]

There is no fix for this test case in 10.6, I added a separate commit that fixes it to this PR.

[svoj]

Removed test case fix, it is in 10.6 now.