Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            dougiamas Martin Dougiamas added a comment -

            Sam can you review this?

            Show
            dougiamas Martin Dougiamas added a comment - Sam can you review this?
            Hide
            samhemelryk 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
            samhemelryk 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
            skodak Petr Skoda added a comment -

            I am adding new issue for the locking improvement backport

            Show
            skodak Petr Skoda added a comment - I am adding new issue for the locking improvement backport
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated, thanks.

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

            Show
            poltawski Dan Poltawski added a comment - Integrated, thanks. Have added the api_change flag to ensure this gets a note in release notes.
            Hide
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            skodak Petr Skoda added a comment -

            arrgh, fixing

            Show
            skodak Petr Skoda added a comment - arrgh, fixing
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - oh, the new enebledblogs fails bloglevel during install, hehe, obviously you did not test your patch either...
            Hide
            abgreeve 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
            abgreeve 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
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  3/Dec/12