Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-34520

purge_sys_t::wait_FTS sleeps 10ms, even if it does not have to wait.

Details

    Description

      The code reads like this

        bool paused;
        do
        {
          latch.wr_lock(SRW_LOCK_CALL);
          paused= m_FTS_paused || (also_sys && m_SYS_paused);
          latch.wr_unlock();
          std::this_thread::sleep_for(std::chrono::milliseconds(10));
        }
        while (paused);
      

      it sleeps. without checking paused, even if paused is false.

      It looks wrong, although perhaps it was intended for some purpose, e.g throttle purge activity.

      Even if it was intended, then it still needs to be taken out of the function loop, and added to where the throttling needs to actually be, with appropritate comment.

      Otherwise it adds 10 ms latency to all of its callers, and to the callers of its callers, which happens more than once during the purge

      e.g in wait_FTS/clone_oldest_view/trx_purge and in wait_FTS/close_and_reopen/trx_attach_undo_recs/trx_purge code paths

      Other thing : taking a write lock latch.wr_lock just to check the variable, and not change anything is this correct? Looks strange at least.

      Attachments

        Issue Links

          Activity

            If this is fixed, I'd reevaluate the change in 10.6 for innodb_purge_batch_size=300 to 1000. I recall having trying out, and having better benchmark results with the old default, but only if this sleep is removed. marko, FYI.

            wlad Vladislav Vaintroub added a comment - If this is fixed, I'd reevaluate the change in 10.6 for innodb_purge_batch_size=300 to 1000. I recall having trying out, and having better benchmark results with the old default, but only if this sleep is removed. marko , FYI.

            This interferes with the testing of MDEV-34515. I came up with a solution that would merge purge_sys.m_SYS_paused and purge_sys.m_FTS_paused into a single std::atomic<uint32_t>. That should remove any need to acquire purge_sys.latch.

            marko Marko Mäkelä added a comment - This interferes with the testing of MDEV-34515 . I came up with a solution that would merge purge_sys.m_SYS_paused and purge_sys.m_FTS_paused into a single std::atomic<uint32_t> . That should remove any need to acquire purge_sys.latch .

            People

              marko Marko Mäkelä
              wlad Vladislav Vaintroub
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.