Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-29198

Delegated transactions not working properly on MySQL

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      A) To be tested under the five supported drivers: mysqli (InnoDB only), postgres, mssql, sqlsrv and oracle
      B) To be tested under 21_STABLE ! (fix is 100% the same in other branches)

      1) Run DB functional tests
      2) TEST: Ignoring other errors, no error should be shown related with transactions (specially test_concurent_transactions ones).

      Show
      A) To be tested under the five supported drivers: mysqli (InnoDB only), postgres, mssql, sqlsrv and oracle B) To be tested under 21_STABLE ! (fix is 100% the same in other branches) 1) Run DB functional tests 2) TEST: Ignoring other errors, no error should be shown related with transactions (specially test_concurent_transactions ones).
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      To reproduce:

      Mysql with Innodb:

      0. A table named onetable has some records in it
      1. Start a delegated transaction
      2. Execute: $DB->delete_records('onetable');
      3. Open a new connection with mysql client
      4. Perform a SELECT over onetable

      What you'll see is that onetable is empty... but it shouldn't because I've not performed a commit on moodle and we're in READ COMMITTED isolation level.

      All records in the table should be returned.

      Rationale:

      Some SQL statements in mysql cause an implicit commit. This should be taken into account because code that relies on transactions can not be working as expected.

      This is what i've found (Mysql+Innodb)

      1. Start a delegated transaction
      2. Execute: $DB->delete_records('onetable');
      ...

      delete_records() optimizes the deletion by issuing a TRUNCATE TABLE statement... in mysql this performs an implicit commit ( http://dev.mysql.com/doc/refman/5.1/en/implicit-commit.html ) so statements executed after delete_records() are not performed atomically nor READ COMMITTED isolated.

      Maybe TRUNCATE TABLE should not be issued inside a transaction... or maybe some warn should be issued... I don't what's the best general approach

      What do yo think?

      Thanks in advance

        Gliffy Diagrams

          Activity

          Hide
          salvetore Michael de Raadt added a comment -

          I'll leave this to Eloy for comment.

          Show
          salvetore Michael de Raadt added a comment - I'll leave this to Eloy for comment.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Well spotted, Juan!

          We should not use the TRUNCATE (DDL) shortcut within transactions at all, 100% agree.

          I'm fixing delete_records() and adding some more unit tests to check that everything works ok after the change.

          Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Well spotted, Juan! We should not use the TRUNCATE (DDL) shortcut within transactions at all, 100% agree. I'm fixing delete_records() and adding some more unit tests to check that everything works ok after the change. Thanks!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Fix & test created. Sent to integration next week, thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Fix & test created. Sent to integration next week, thanks!
          Hide
          nebgor Aparup Banerjee added a comment -

          Well spotted! This has been integrated.

          Show
          nebgor Aparup Banerjee added a comment - Well spotted! This has been integrated.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hey guys,

          Just noting I have tested this in both MySQL and Postgres - both passed.
          I don't have any Windows VM's here so I can't test the others sorry.
          Perhaps Apu you could test what you can over there or get someone else in the office to.

          Cheer
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hey guys, Just noting I have tested this in both MySQL and Postgres - both passed. I don't have any Windows VM's here so I can't test the others sorry. Perhaps Apu you could test what you can over there or get someone else in the office to. Cheer Sam
          Hide
          nebgor Aparup Banerjee added a comment -

          Thanks Sam, will do. good thing i've just setup my freetds

          Show
          nebgor Aparup Banerjee added a comment - Thanks Sam, will do. good thing i've just setup my freetds
          Hide
          nebgor Aparup Banerjee added a comment -

          I'm not sure if this is my specific setup (I've just set this up recently) but getting an error under exactly test_concurent_transactions (ps: note concurrent spell err)

          while Running tests on: Current database (native/mssql)

          Exception: lib/dml/simpletest/testdml.php / ► dml_test / ► test_concurent_transactions
          Unexpected exception of type [dml_read_exception] with message [Error reading from database] in [/home/aparup/mcode/21/mssql/lib/dml/moodle_database.php line 394]

          Debug info:

          Changed database context to '21_mssql'.
          SELECT COUNT('x') FROM mdl_unit_table
          [array (
          )]

          line 255 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end()
          line 710 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
          line 739 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql()
          line 1290 of /lib/dml/moodle_database.php: call to mssql_native_moodle_database->get_records_sql()
          line 1365 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
          line 1536 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql()
          line 1519 of /lib/dml/moodle_database.php: call to moodle_database->count_records_sql()
          line 1502 of /lib/dml/moodle_database.php: call to moodle_database->count_records_select()
          line 3985 of /lib/dml/simpletest/testdml.php: call to moodle_database->count_records()
          line ... of ...

          Show
          nebgor Aparup Banerjee added a comment - I'm not sure if this is my specific setup (I've just set this up recently) but getting an error under exactly test_concurent_transactions (ps: note concurrent spell err) while Running tests on: Current database (native/mssql) Exception: lib/dml/simpletest/testdml.php / ► dml_test / ► test_concurent_transactions Unexpected exception of type [dml_read_exception] with message [Error reading from database] in [/home/aparup/mcode/21/mssql/lib/dml/moodle_database.php line 394] Debug info: Changed database context to '21_mssql'. SELECT COUNT('x') FROM mdl_unit_table [array ( )] line 255 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end() line 710 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end() line 739 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql() line 1290 of /lib/dml/moodle_database.php: call to mssql_native_moodle_database->get_records_sql() line 1365 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() line 1536 of /lib/dml/moodle_database.php: call to moodle_database->get_field_sql() line 1519 of /lib/dml/moodle_database.php: call to moodle_database->count_records_sql() line 1502 of /lib/dml/moodle_database.php: call to moodle_database->count_records_select() line 3985 of /lib/dml/simpletest/testdml.php: call to moodle_database->count_records() line ... of ...
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          (as commented via chat, surely it can be a matter of running one stock FreeTDS 0.82 version, we need a patched one to be used instead. Like the windows one available to download @ Moodle Docs, or any package building against patched one)

          Surely http://ibiblio.org/pub/Linux/ALPHA/freetds/old/0.82/freetds-patched.tgz is the one to pick and try with in case of own-compilation.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - (as commented via chat, surely it can be a matter of running one stock FreeTDS 0.82 version, we need a patched one to be used instead. Like the windows one available to download @ Moodle Docs, or any package building against patched one) Surely http://ibiblio.org/pub/Linux/ALPHA/freetds/old/0.82/freetds-patched.tgz is the one to pick and try with in case of own-compilation. Ciao
          Hide
          nebgor Aparup Banerjee added a comment -

          spoke to Eloy, apparently theres a patched version of freetds to use that takes care of many issues. (possibly the above testing issue too)

          http://ibiblio.org/pub/Linux/ALPHA/freetds/old/0.82/freetds-patched.tgz

          i'll re-install freetds with this and test again ..

          Show
          nebgor Aparup Banerjee added a comment - spoke to Eloy, apparently theres a patched version of freetds to use that takes care of many issues. (possibly the above testing issue too) http://ibiblio.org/pub/Linux/ALPHA/freetds/old/0.82/freetds-patched.tgz i'll re-install freetds with this and test again ..
          Hide
          nebgor Aparup Banerjee added a comment -

          Hello, i've updated my freetds build to that patched version but still getting the above exception .
          current freetds version (put from 'tsql -C'):
          Compile-time settings (established with the "configure" script)
          Version: freetds v0.82.1.dev.20110409
          freetds.conf directory: /usr/local/freetds/etc
          MS db-lib source compatibility: yes
          Sybase binary compatibility: no
          Thread safety: yes
          iconv library: yes
          TDS version: 5.0
          iODBC: no
          unixodbc: no

          Show
          nebgor Aparup Banerjee added a comment - Hello, i've updated my freetds build to that patched version but still getting the above exception . current freetds version (put from 'tsql -C'): Compile-time settings (established with the "configure" script) Version: freetds v0.82.1.dev.20110409 freetds.conf directory: /usr/local/freetds/etc MS db-lib source compatibility: yes Sybase binary compatibility: no Thread safety: yes iconv library: yes TDS version: 5.0 iODBC: no unixodbc: no
          Hide
          nebgor Aparup Banerjee added a comment -

          ps: this does work for SQLSRV (& oracle btw) so i'll pass this as its likely a freetds issue i'm facing.

          Show
          nebgor Aparup Banerjee added a comment - ps: this does work for SQLSRV (& oracle btw) so i'll pass this as its likely a freetds issue i'm facing.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories updated with your gorgeous code. Many thanks!

          Closing and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Oct/11