Moodle
  1. Moodle
  2. MDL-36211

improve perf by not locking guest and not-logged-in sessions and skipping record updates when not necessary

    Details

    • Testing Instructions:
      Hide

      1/ try working with moodle as not-logged-in-user (mostly frontapge only)
      2/ try working as guest
      3/ try logging in, working, logging out, login in, ...
      4/ enable new setting $CFG->sessionlockloggedinonly = 1; and repeat testing 1-3

      Show
      1/ try working with moodle as not-logged-in-user (mostly frontapge only) 2/ try working as guest 3/ try logging in, working, logging out, login in, ... 4/ enable new setting $CFG->sessionlockloggedinonly = 1; and repeat testing 1-3
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w47_MDL-36211_m24_sesslocking
    • Rank:
      44991

      Description

      I suppose it should be relatively safe to not lock guest and not-logged-in sessions.

      I am going to implement it as a config.php only setting for now.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment - - edited

          While testing this I have also added code that tries to non update session record if the data did not change. This should help with performance but it may also prevent race conditions when locking is disabled.

          Show
          Petr Škoda added a comment - - edited While testing this I have also added code that tries to non update session record if the data did not change. This should help with performance but it may also prevent race conditions when locking is disabled.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi Petr,

          I'm not 100% on this, it would be good to have more eyes on it.

          1. I'm concerned about adding an additional DB read on this contended table to achieve this goal
          2. error_log('Can read session record');
          Show
          Dan Poltawski added a comment - Hi Petr, I'm not 100% on this, it would be good to have more eyes on it. I'm concerned about adding an additional DB read on this contended table to achieve this goal error_log('Can read session record');
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Petr Škoda added a comment -

          hi,

          1. the extra read fixes a serious problem in the session locking logic, we should probably consider backporting it. The rest of the changes should compensate for that extra query.
          2. fixed, thanks a lot

          Show
          Petr Škoda added a comment - hi, 1. the extra read fixes a serious problem in the session locking logic, we should probably consider backporting it. The rest of the changes should compensate for that extra query. 2. fixed, thanks a lot
          Hide
          Martin Dougiamas added a comment -

          Sam can you review this?

          Show
          Martin Dougiamas added a comment - Sam can you review this?
          Hide
          Sam Hemelryk added a comment -

          Hi Petr,

          Gets a +1 from me, certainly I think it is worth backporting the bug fix portion of this as well, it should be easy enough to and is quite a gotcha.
          I'll leave it up to you as to whether it's worth doing that as a separate issue or whether we should just plant them in here.
          Either way good to go for integration when you are ready.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr, Gets a +1 from me, certainly I think it is worth backporting the bug fix portion of this as well, it should be easy enough to and is quite a gotcha. I'll leave it up to you as to whether it's worth doing that as a separate issue or whether we should just plant them in here. Either way good to go for integration when you are ready. Many thanks Sam
          Hide
          Petr Škoda added a comment -

          I am adding new issue for the locking improvement backport

          Show
          Petr Škoda added a comment - I am adding new issue for the locking improvement backport
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks.

          Have added the api_change flag to ensure this gets a note in release notes.

          Show
          Dan Poltawski added a comment - Integrated, thanks. Have added the api_change flag to ensure this gets a note in release notes.
          Hide
          Dan Poltawski added a comment -

          Failing this, its breaking install:

          PHP Fatal error:  Call to undefined function session_get_instance() in /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php on line 346
          PHP Stack trace:
          PHP   1. {main}() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:0
          PHP   2. moodle_database->__destruct() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:183
          PHP   3. sqlsrv_native_moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:146
          PHP   4. moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/sqlsrv_native_moodle_database.php:232
          
          Fatal error: Call to undefined function session_get_instance() in /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php on line 346
          
          Call Stack:
              0.0015     993832   1. {main}() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:0
              0.0931   25229296   2. moodle_database->__destruct() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:183
              0.0931   25229296   3. sqlsrv_native_moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:146
              0.0931   25229296   4. moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/sqlsrv_native_moodle_database.php:232
          
          Error installing master to test upgrade
          
          Show
          Dan Poltawski added a comment - Failing this, its breaking install: PHP Fatal error: Call to undefined function session_get_instance() in /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php on line 346 PHP Stack trace: PHP 1. {main}() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:0 PHP 2. moodle_database->__destruct() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:183 PHP 3. sqlsrv_native_moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:146 PHP 4. moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/sqlsrv_native_moodle_database.php:232 Fatal error: Call to undefined function session_get_instance() in /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php on line 346 Call Stack: 0.0015 993832 1. {main}() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:0 0.0931 25229296 2. moodle_database->__destruct() /Users/Shared/Jenkins/Home/git_repositories/master/admin/cli/install.php:183 0.0931 25229296 3. sqlsrv_native_moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/moodle_database.php:146 0.0931 25229296 4. moodle_database->dispose() /Users/Shared/Jenkins/Home/git_repositories/master/lib/dml/sqlsrv_native_moodle_database.php:232 Error installing master to test upgrade
          Hide
          Dan Poltawski added a comment -

          I'm waiting to hear about Petr for this as i'm not certain if the simple require is a good solution here, install in an unknown state where sessionlib may not be good. Its obviously not a tested path.

          Show
          Dan Poltawski added a comment - I'm waiting to hear about Petr for this as i'm not certain if the simple require is a good solution here, install in an unknown state where sessionlib may not be good. Its obviously not a tested path.
          Hide
          Petr Škoda added a comment -

          arrgh, fixing

          Show
          Petr Škoda added a comment - arrgh, fixing
          Hide
          Petr Škoda added a comment -

          fix pushed to my repo, sorry, I was concentrating on normal page access and upgrades and forgot the installer...

          I have some other problems with installer now, fixing...

          Show
          Petr Škoda added a comment - fix pushed to my repo, sorry, I was concentrating on normal page access and upgrades and forgot the installer... I have some other problems with installer now, fixing...
          Hide
          Petr Škoda added a comment -

          oh, the new enebledblogs fails bloglevel during install, hehe, obviously you did not test your patch either...

          Show
          Petr Škoda added a comment - oh, the new enebledblogs fails bloglevel during install, hehe, obviously you did not test your patch either...
          Hide
          Adrian Greeve added a comment -

          Tested on the master integration branch.
          I tried working as a guest and not-logged-in-user.
          I logged out and in again.
          I then enabled the $CFG->sessionlockloggedinonly setting.
          I repeated the above steps.
          I didn't encounter any issues.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the master integration branch. I tried working as a guest and not-logged-in-user. I logged out and in again. I then enabled the $CFG->sessionlockloggedinonly setting. I repeated the above steps. I didn't encounter any issues. Test passed.
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: