Moodle
  1. Moodle
  2. MDL-30242

Prevent session lock waits from stalling server processes forever (backport of MDL-30026)

    Details

    • Rank:
      26513

      Description

      If everything goes ok with MDL-30026 under master (2.2), and no regressions are reported, consider backporting it to 21_STABLE (no way to 20_STABLE). It makes database session locking better.

      Ciao

        Issue Links

          Activity

          Hide
          Tom Lanyon added a comment - - edited

          This is important (for me, from a sysadmin perspective) for existing 2.1 production sites. Would appreciate someone adding netspot and/or partner label to this one if possible.

          Show
          Tom Lanyon added a comment - - edited This is important (for me, from a sysadmin perspective) for existing 2.1 production sites. Would appreciate someone adding netspot and/or partner label to this one if possible.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding Petr here, I don't feel myself strong enough to backport MDL-30026 to 21_STABLE. Which are your thoughts, Petr?

          With 2.2 out since 4 months ago... Tom, does this continue being important?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Adding Petr here, I don't feel myself strong enough to backport MDL-30026 to 21_STABLE. Which are your thoughts, Petr? With 2.2 out since 4 months ago... Tom, does this continue being important? Ciao
          Hide
          Tom Lanyon added a comment -

          Eloy, yes - clients will be on 2.1 for some time and this is a pretty significant performance/availability issue.

          Show
          Tom Lanyon added a comment - Eloy, yes - clients will be on 2.1 for some time and this is a pretty significant performance/availability issue.
          Hide
          Petr Škoda added a comment -

          Hi, I can crete patch, but somebody has to test it on real systems before it gets integrated. Are you going to help?

          Show
          Petr Škoda added a comment - Hi, I can crete patch, but somebody has to test it on real systems before it gets integrated. Are you going to help?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I'm happy to run-test it under the 4DBs if needed, np here.

          And I bet Tom will be able to do some real testing in some real site with real load. Tom?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I'm happy to run-test it under the 4DBs if needed, np here. And I bet Tom will be able to do some real testing in some real site with real load. Tom? Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Also, we have MDL-32236, uhm.. somehow related.

          Show
          Eloy Lafuente (stronk7) added a comment - Also, we have MDL-32236 , uhm.. somehow related.
          Hide
          Petr Škoda added a comment -

          Hmm, no idea what the problem is in MDL-32236 and I have no way to find out because windows is not on my test server, sorry.

          Show
          Petr Škoda added a comment - Hmm, no idea what the problem is in MDL-32236 and I have no way to find out because windows is not on my test server, sorry.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (delaying thins for next week, I got stuck with other DB issues and integration and was not able to review this with all the required care)

          Show
          Eloy Lafuente (stronk7) added a comment - (delaying thins for next week, I got stuck with other DB issues and integration and was not able to review this with all the required care)
          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
          Eloy Lafuente (stronk7) added a comment -

          Ping, seems the branch is not available. Halting this for some hours...

          Show
          Eloy Lafuente (stronk7) added a comment - Ping, seems the branch is not available. Halting this for some hours...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Note to testers: This has been tested under mysql, pgsql, mssql and oracle (and there are not session/locks tests failing). Only sqlsrv driver needs testing.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note to testers: This has been tested under mysql, pgsql, mssql and oracle (and there are not session/locks tests failing). Only sqlsrv driver needs testing.
          Hide
          Rajesh Taneja added a comment -

          No error related to "test_session lock.

          Following error/exception happen:

          Exception: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql
          Unexpected PHP error [mssql_data_seek(): Bad row offset] severity [2] in [/var/www/test/lib/dml/mssql_native_moodle_database.php line 720] 
          
          Exception: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql
          Unexpected PHP error [mssql_data_seek(): Bad row offset] severity [2] in [/var/www/test/lib/dml/mssql_native_moodle_database.php line 720] 
          
          Fail: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql
          Equal expectation fails because [Integer: 0] differs from [Integer: 4] by 4 at [/var/www/test/lib/dml/simpletest/testdml.php line 1471] 
          
          Fail: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql
          Equal expectation fails because [Integer: 0] differs from [Integer: 4] by 4 at [/var/www/test/lib/dml/simpletest/testdml.php line 1475] 
          
          Show
          Rajesh Taneja added a comment - No error related to "test_session lock. Following error/exception happen: Exception: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql Unexpected PHP error [mssql_data_seek(): Bad row offset] severity [2] in [/ var /www/test/lib/dml/mssql_native_moodle_database.php line 720] Exception: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql Unexpected PHP error [mssql_data_seek(): Bad row offset] severity [2] in [/ var /www/test/lib/dml/mssql_native_moodle_database.php line 720] Fail: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql Equal expectation fails because [ Integer : 0] differs from [ Integer : 4] by 4 at [/ var /www/test/lib/dml/simpletest/testdml.php line 1471] Fail: lib/dml/simpletest/testdml.php / ► dml_test / ► test_get_records_sql Equal expectation fails because [ Integer : 0] differs from [ Integer : 4] by 4 at [/ var /www/test/lib/dml/simpletest/testdml.php line 1475]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh, 2 little things:

          1) The "data seek" exceptions are "normal" under mssql. Can be ignored safely. The other 2 "0 differs from 4" surprised me, so I went to look and it seems that you have performed the testrun into some incorrect branch (master?). Note this issue is 21_STABLE only and tests must be performed there.

          2) More yet, the tests must be performed with "sqlsrv", not with "mssql". It's the only database not tested here.

          So, I'm sending this back to "waiting for testing" status... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, 2 little things: 1) The "data seek" exceptions are "normal" under mssql. Can be ignored safely. The other 2 "0 differs from 4" surprised me, so I went to look and it seems that you have performed the testrun into some incorrect branch (master?). Note this issue is 21_STABLE only and tests must be performed there. 2) More yet, the tests must be performed with "sqlsrv", not with "mssql". It's the only database not tested here. So, I'm sending this back to "waiting for testing" status... ciao
          Hide
          Rajesh Taneja added a comment -

          grr.. Thanks Eloy...

          Testing on 21 now.

          Show
          Rajesh Taneja added a comment - grr.. Thanks Eloy... Testing on 21 now.
          Hide
          Rajesh Taneja added a comment -

          My Bad..
          I was running this on mssql not sqlsrv... Which is required for this test.
          Will request Michael to run this, as I don't have sqlsrv setup.

          Show
          Rajesh Taneja added a comment - My Bad.. I was running this on mssql not sqlsrv... Which is required for this test. Will request Michael to run this, as I don't have sqlsrv setup.
          Hide
          Rajesh Taneja added a comment -

          Requesting Michael to test this.
          Sorry, I don't have sqlsrv setup.

          Show
          Rajesh Taneja added a comment - Requesting Michael to test this. Sorry, I don't have sqlsrv setup.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, we need to test that under (windows-only) Microsoft's sqlsrv driver.

          In any case, Rajesh, something is still bad/missing in your "mssql" installation. You should not get failures in test_reset_sequence() nor test_change_field_type() nor test_concurent_transactions().

          Note that does not affect result of testing. I'm passing here the "mssql" ones properly, with the expected (3 exceptions + 4 failures). But the extra 3 failures you're getting are not expected at all.

          Waiting for Michael results using "sqlsrv"...

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, we need to test that under (windows-only) Microsoft's sqlsrv driver. In any case, Rajesh, something is still bad/missing in your "mssql" installation. You should not get failures in test_reset_sequence() nor test_change_field_type() nor test_concurent_transactions(). Note that does not affect result of testing. I'm passing here the "mssql" ones properly, with the expected (3 exceptions + 4 failures). But the extra 3 failures you're getting are not expected at all. Waiting for Michael results using "sqlsrv"...
          Hide
          Rajesh Taneja added a comment -

          Thanks Eloy...
          It seems db alter command was not executed.
          Will wait for Aparup to come back and see how it goes (As he created MSSQl account for me.)

          Show
          Rajesh Taneja added a comment - Thanks Eloy... It seems db alter command was not executed. Will wait for Aparup to come back and see how it goes (As he created MSSQl account for me.)
          Hide
          Michael de Raadt added a comment -

          I tried to run the PHPUnit tests under SQL Server on Windows, but it stalled.

          I will try to run it again overnight and see if there is any difference.

          Show
          Michael de Raadt added a comment - I tried to run the PHPUnit tests under SQL Server on Windows, but it stalled. I will try to run it again overnight and see if there is any difference.
          Hide
          Petr Škoda added a comment -

          I had success with sqlsrv driver version 3.0 only, the older 2.0 version drivers takes down PHP process randomly.

          Show
          Petr Škoda added a comment - I had success with sqlsrv driver version 3.0 only, the older 2.0 version drivers takes down PHP process randomly.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          PHPUnit tests? it is 21_STABLE so only simpletest should be there.

          The most I see all your continue problems runnning mssql and sqlsrv, the most I think there is something missing there, in your SQL*Server server. I've pointed Rajesh about some "ALTER DATABASE..." statements required on db creation. They are all documented in the Docs. Let's see if that solves your problems there.

          In the mean time I'm going to ask Petr for testing this using the sqlsrv driver. And, if I'm able to do so I'll try to have here also one windows instance with sqlsrv available.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - PHPUnit tests? it is 21_STABLE so only simpletest should be there. The most I see all your continue problems runnning mssql and sqlsrv, the most I think there is something missing there, in your SQL*Server server. I've pointed Rajesh about some "ALTER DATABASE..." statements required on db creation. They are all documented in the Docs. Let's see if that solves your problems there. In the mean time I'm going to ask Petr for testing this using the sqlsrv driver. And, if I'm able to do so I'll try to have here also one windows instance with sqlsrv available. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ah, lol, just saved my comment and saw Petr's one 2 mins before, hehe.

          So perhaps we should be recommending to use the 3.0 driver exclusively?

          In any case, I'd pass this. Knowing the involved code, it's a 100% copycat of the one present in 22 and master, so I cannot imagine any reason to avoid this to pass.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - ah, lol, just saved my comment and saw Petr's one 2 mins before, hehe. So perhaps we should be recommending to use the 3.0 driver exclusively? In any case, I'd pass this. Knowing the involved code, it's a 100% copycat of the one present in 22 and master, so I cannot imagine any reason to avoid this to pass. Ciao
          Hide
          Petr Škoda added a comment -

          I tried a few installs using 3.0 driver and different browser, it failed the first time, now it works 100% here in functional tests (I do my little hacking for case sensitiveness to make it work with case sensitive collation, it is already in master). So I guess this patch does not introduce any new problems.

          Show
          Petr Škoda added a comment - I tried a few installs using 3.0 driver and different browser, it failed the first time, now it works 100% here in functional tests (I do my little hacking for case sensitiveness to make it work with case sensitive collation, it is already in master). So I guess this patch does not introduce any new problems.
          Hide
          Michael de Raadt added a comment -

          Looks like I should have been paying more attention. Raj asked me to test this one also and I assumed he wanted me to run the PHPUnit tests with SQL Server.

          I set up a 2.1 instance with SQL Server and I am able to run the Functional DB tests with only one fail: dml_test / ► test_sql_binary_equal and this has to do with my collation, which is case insensitive.

          I'm using the version 3 drivers (eg. php_sqlsrv_53_ts_vc9.dll).

          So that looks consistent to me.

          Show
          Michael de Raadt added a comment - Looks like I should have been paying more attention. Raj asked me to test this one also and I assumed he wanted me to run the PHPUnit tests with SQL Server. I set up a 2.1 instance with SQL Server and I am able to run the Functional DB tests with only one fail: dml_test / ► test_sql_binary_equal and this has to do with my collation, which is case insensitive. I'm using the version 3 drivers (eg. php_sqlsrv_53_ts_vc9.dll). So that looks consistent to me.
          Hide
          Michael de Raadt added a comment -

          Test result: Success

          Tested on 2.1 with SQL Server.

          Show
          Michael de Raadt added a comment - Test result: Success Tested on 2.1 with SQL Server.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks all!

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks all!
          Hide
          Dan Poltawski added a comment -

          Bonza mate!

          Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

          Hooroo

          Show
          Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

            People

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

              Dates

              • Created:
                Updated:
                Resolved: