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

Remove the deadcode from srv_master_thread() and srv_active_wake_master_thread_low()

Details

    Description

      Deadcode is present in srv_master_thread() and srv_active_wake_master_thread_low().
      srv_master_thread() doesn't even wait for any event. So there is no need to wake the
      master thread with setting the event.

      This issue caused by mdev-16125:

      commit fe95cb2e40d73adbbe88efa0e7f48c055ccd042f
      Author: Thirunarayanan Balathandayuthapani <thiru@mariadb.com>
      Date:   Wed May 9 19:00:30 2018 +0530
       
          MDEV-16125  Shutdown crash when innodb_force_recovery >= 2
       

      Attachments

        Issue Links

          Activity

            thiru Thirunarayanan Balathandayuthapani added a comment - Attaching the patch to remove the deadcode MDEV-20638-10.2-e40ed0e881690ed072c28c0f4080a3cfb83e5d73v1.patch

            This looks good to me, but I think that we can remove even more code:

            • Remove srv_wake_master_thread() and invoke srv_inc_activity_count() instead.
            • Simplify srv_master_thread further. Because srv_shutdown_state cannot change back to SRV_SHUTDOWN_NONE while the thread exists, the switch can be removed.
            • Make parameter of srv_check_activity() in/out, removing the need of most calls to srv_get_activity_count().
            • Do we need SRV_MASTER_SLOT and SRV_MASTER at all? What is the srv_reserve_slot(SRV_MASTER) good for?

            Idea for srv_master_thread simplification:

            	while (srv_shutdown_state == SRV_SHUTDOWN_NONE) {
            	}
            	ut_ad(srv_shutdown_state == SRV_SHUTDOWN_EXIT_THREADS || srv_shutdown_state == SRV_SHUTDOWN_CLEANUP);
            	os_thread_exit();
            }
            

            marko Marko Mäkelä added a comment - This looks good to me, but I think that we can remove even more code: Remove srv_wake_master_thread() and invoke srv_inc_activity_count() instead. Simplify srv_master_thread further. Because srv_shutdown_state cannot change back to SRV_SHUTDOWN_NONE while the thread exists, the switch can be removed. Make parameter of srv_check_activity() in/out, removing the need of most calls to srv_get_activity_count() . Do we need SRV_MASTER_SLOT and SRV_MASTER at all? What is the srv_reserve_slot(SRV_MASTER) good for? Idea for srv_master_thread simplification: while (srv_shutdown_state == SRV_SHUTDOWN_NONE) { … } ut_ad(srv_shutdown_state == SRV_SHUTDOWN_EXIT_THREADS || srv_shutdown_state == SRV_SHUTDOWN_CLEANUP); … os_thread_exit(); }

            Fixed all the review comments and patch is in bb-10.2-thiru. We need SRV_MASTER_SLOT, SRV_MASTER & srv_reserve_slot(SRV_MASTER) to make sure
            that master thread exits before assigning SRV_SHUTDOWN_FLUSH_PHASE

            thiru Thirunarayanan Balathandayuthapani added a comment - Fixed all the review comments and patch is in bb-10.2-thiru. We need SRV_MASTER_SLOT, SRV_MASTER & srv_reserve_slot(SRV_MASTER) to make sure that master thread exits before assigning SRV_SHUTDOWN_FLUSH_PHASE

            Thank you for the update. I sent some comments. I think that any potentially functional changes must be documented in the commit message.

            marko Marko Mäkelä added a comment - Thank you for the update. I sent some comments. I think that any potentially functional changes must be documented in the commit message.

            This looks OK, but you forgot to remove INNOBASE_WAKE_INTERVAL from extra/mariabackup/xtrabackup.cc and storage/innobase/handler/ha_innodb.cc. I can also spot some inadvertent changes to white space. The commit message should mention that srv_master_thread() does not need to call srv_resume_thread() and therefore there is no need to wake up the thread.

            marko Marko Mäkelä added a comment - This looks OK, but you forgot to remove INNOBASE_WAKE_INTERVAL from extra/mariabackup/xtrabackup.cc and storage/innobase/handler/ha_innodb.cc . I can also spot some inadvertent changes to white space. The commit message should mention that srv_master_thread() does not need to call srv_resume_thread() and therefore there is no need to wake up the thread.

            People

              thiru Thirunarayanan Balathandayuthapani
              thiru Thirunarayanan Balathandayuthapani
              Votes:
              0 Vote for this issue
              Watchers:
              2 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.