Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Database SQL/XMLDB
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      24322

      Description

      This topic was brought forward by Tim in http://moodle.org/mod/forum/discuss.php?d=135847

      I personally do not think we should start using the transactions in moodle core now, but in any case I already considered the nested transactions last year. At that time it could not be done because we were not yet using exceptions.

      Attaching patch that adds nested transactions without API changes, includes unittests.

      1. delegated_transactions_20091103.patch
        66 kB
        Petr Škoda
      2. delegated_transactions_20091104_2.patch
        69 kB
        Petr Škoda
      3. delegated_transactions_20091104_3.patch
        70 kB
        Petr Škoda
      4. delegated_transactions_20091104.patch
        68 kB
        Petr Škoda
      5. nested_transactions_3.patch
        19 kB
        Petr Škoda
      1. TransactionsAndExceptionsFlow.png
        41 kB

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          -1 (note the vote isn't against the implementation but against the the concept, nested transactions, like oil and water!) :-P

          Show
          Eloy Lafuente (stronk7) added a comment - - edited -1 (note the vote isn't against the implementation but against the the concept, nested transactions, like oil and water!) :-P
          Hide
          Petr Škoda added a comment -

          :-D

          Show
          Petr Škoda added a comment - :-D
          Hide
          Petr Škoda added a comment -

          my +-0

          Show
          Petr Škoda added a comment - my +-0
          Hide
          Petr Škoda added a comment -

          my +1 for single level transactions allowed only in some layers:

          • external API functions
          • restore

          If we use it in external API we can not use it in enrol and auth any more, because they are supposed to use the external API too.

          If we decide to use my patch it can be done later in the stable 2.x because it does not change API. If we want Tim's patch it has to be done ASAP.

          Show
          Petr Škoda added a comment - my +1 for single level transactions allowed only in some layers: external API functions restore If we use it in external API we can not use it in enrol and auth any more, because they are supposed to use the external API too. If we decide to use my patch it can be done later in the stable 2.x because it does not change API. If we want Tim's patch it has to be done ASAP.
          Hide
          Martín Langhoff added a comment -

          +1 for the tests, -1 for the implementation.

          I prefer Tim's implementation (with an added warning on nested transactions) because

          • it keeps track of the ordering of potentially nested transactions "out of order" commits or rollbacks will be caught
          • it requires the calling code to keep around a "cookie" which leads to a saner code flow
          Show
          Martín Langhoff added a comment - +1 for the tests, -1 for the implementation. I prefer Tim's implementation (with an added warning on nested transactions) because it keeps track of the ordering of potentially nested transactions "out of order" commits or rollbacks will be caught it requires the calling code to keep around a "cookie" which leads to a saner code flow
          Hide
          Martin Dougiamas added a comment -

          Eloy, I'm leaving this to you because you are the best database expert I know. Can you think about all the angles proposed here and make a recommendation so we can get this in ASAP?

          Show
          Martin Dougiamas added a comment - Eloy, I'm leaving this to you because you are the best database expert I know. Can you think about all the angles proposed here and make a recommendation so we can get this in ASAP?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Here it's my opinion:

          A) About the concept.

          1. nested transactions is a fake concept. They don't exist. Point. I would name them "moodle_transaction" or "smart_transaction" or "logical_transaction".
          2. also the "commit" and/or "rollback" concepts, when applied to those logical transactions, don't mean data will be really committed nor roll-backed (that is delayed until the "top" (the real DB transaction) happens). So I would try to name them in another way or, at least (I understand that "commit" and "rollback" are good terms), explain clearly how they are going to work in Docs, stating that, other requests cannot rely on data being already committed and so on.
          3. code must not rely on commit/rollback status to perform any special action, like trying again, using an alternate method or other techniques. At the same time, we cannot perform one commit()/rollback() based on arbitrary conditions to achieve any result (because it's nested-dependent).
          4. we must start recommending to MySQL about the benefits of switching to InnoDB engine (recommendation in environmental stuff, perhaps some day will become requirement).

          I think it's easy to fulfil these 4 points.

          B) About the implementation.

          1. I think that everything must happen within the $DB (starting the transaction, committing and roll-backing it. No objects go out from $DB at all.
          2. Commit/rollback always happen at top level, when the outer logical transaction is handled.
          3. Every block of code using logical exceptions must follow one clear pattern (see below for the proposed pattern). We need (must!) to create one utility to check for bad uses
          4. About the transaction token (to detect where "unclosed" logical transactions are) I think it's nice but not necessary (as far as OU/Mahara/ADOdb have been using similar solutions without problems since ages ago). If there is one error it will be shown immediately when programming. More if the pattern in the point above is 100% endorsed by programers.
          5. About the name of the $DB public API, I'm more about "commit" and "rollback" over begin_sql() and commit_sql()... (although, as stated in A2 above, if somebody can find some names not so explicit, better for me).
          6. Once one rollback has been issued at any level, the transaction is put in "rollback-able" status, and nobody (intermediate transactions) will be able to change that until the top one performs the real rollback. Note commit behaviour is different, so it can be changed by one outer level transaction.
          7. If, at the end of the process there are unclosed logical transactions, debugging levels and print-out errors must be honoured and information must be logged/displayed as necessary. Rollback will be performed and "transaction exception" thrown.
          8. Any other detectable error (try to close too many (non existing) logical transactions, try to commit already marked for rollback transaction... whatever) should also throw one "transaction exception".

          And that's all. If anybody is against any of these 8 points, please comment ASAP.

          Here it's how the logical transactions should (must) be used:

          try {
              $DB->start_transaction();
              // Execute a bunch of SQLs here
              // trying to keep out other things like files and friends as possible
              $DB->commit_transaction();
          } catch (dml_exception $e) {
              $DB->rollback_transaction();
              throw $e
          } catch (ddl_exception $e) { // Just in case we want to play with transactions at install/upgrade
              $DB->rollback_transaction();
              throw $e
          } catch (exception $e) { // Only if you want to catch other exceptions
              // Do whatever you want, unrelated to transactions 100%
          }
          

          That's all. With the 12 points agreed I'll say +1.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Here it's my opinion: A) About the concept. nested transactions is a fake concept. They don't exist. Point. I would name them "moodle_transaction" or "smart_transaction" or "logical_transaction". also the "commit" and/or "rollback" concepts, when applied to those logical transactions, don't mean data will be really committed nor roll-backed (that is delayed until the "top" (the real DB transaction) happens). So I would try to name them in another way or, at least (I understand that "commit" and "rollback" are good terms), explain clearly how they are going to work in Docs, stating that, other requests cannot rely on data being already committed and so on. code must not rely on commit/rollback status to perform any special action, like trying again, using an alternate method or other techniques. At the same time, we cannot perform one commit()/rollback() based on arbitrary conditions to achieve any result (because it's nested-dependent). we must start recommending to MySQL about the benefits of switching to InnoDB engine (recommendation in environmental stuff, perhaps some day will become requirement). I think it's easy to fulfil these 4 points. B) About the implementation. I think that everything must happen within the $DB (starting the transaction, committing and roll-backing it. No objects go out from $DB at all. Commit/rollback always happen at top level, when the outer logical transaction is handled. Every block of code using logical exceptions must follow one clear pattern (see below for the proposed pattern). We need (must!) to create one utility to check for bad uses About the transaction token (to detect where "unclosed" logical transactions are) I think it's nice but not necessary (as far as OU/Mahara/ADOdb have been using similar solutions without problems since ages ago). If there is one error it will be shown immediately when programming. More if the pattern in the point above is 100% endorsed by programers. About the name of the $DB public API, I'm more about "commit" and "rollback" over begin_sql() and commit_sql()... (although, as stated in A2 above, if somebody can find some names not so explicit, better for me). Once one rollback has been issued at any level, the transaction is put in "rollback-able" status, and nobody (intermediate transactions) will be able to change that until the top one performs the real rollback. Note commit behaviour is different, so it can be changed by one outer level transaction. If, at the end of the process there are unclosed logical transactions, debugging levels and print-out errors must be honoured and information must be logged/displayed as necessary. Rollback will be performed and "transaction exception" thrown. Any other detectable error (try to close too many (non existing) logical transactions, try to commit already marked for rollback transaction... whatever) should also throw one "transaction exception". And that's all. If anybody is against any of these 8 points, please comment ASAP. Here it's how the logical transactions should (must) be used: try { $DB->start_transaction(); // Execute a bunch of SQLs here // trying to keep out other things like files and friends as possible $DB->commit_transaction(); } catch (dml_exception $e) { $DB->rollback_transaction(); throw $e } catch (ddl_exception $e) { // Just in case we want to play with transactions at install/upgrade $DB->rollback_transaction(); throw $e } catch (exception $e) { // Only if you want to catch other exceptions // Do whatever you want, unrelated to transactions 100% } That's all. With the 12 points agreed I'll say +1.
          Hide
          Petr Škoda added a comment -

          Surprisingly I have to agree with Eloy again

          Show
          Petr Škoda added a comment - Surprisingly I have to agree with Eloy again
          Hide
          Petr Škoda added a comment -

          begin_transaction(), commit_transaction() and rollback_transaction() are my favourites

          Show
          Petr Škoda added a comment - begin_transaction(), commit_transaction() and rollback_transaction() are my favourites
          Hide
          Penny Leach added a comment -

          That all sounds good for me, except I wonder if we can do something a bit easier for programmers, like some wrapper function, or something. Just to avoid always having to catch at least 2 and often 3 different exceptions.

          If php did proper grown up programming constraints , we could do something like

          $DB->transaction_wrapper(

          { /* block containing all the statements we need */ }

          );

          And that could handle the catching/ throwing of the first 2, but we can't easily do that in php, and any workarounds are probably ugly as hell.

          If anyone has any better ideas, that would be awesome.

          Show
          Penny Leach added a comment - That all sounds good for me, except I wonder if we can do something a bit easier for programmers, like some wrapper function, or something. Just to avoid always having to catch at least 2 and often 3 different exceptions. If php did proper grown up programming constraints , we could do something like $DB->transaction_wrapper( { /* block containing all the statements we need */ } ); And that could handle the catching/ throwing of the first 2, but we can't easily do that in php, and any workarounds are probably ugly as hell. If anyone has any better ideas, that would be awesome.
          Hide
          Penny Leach added a comment -

          i also mildly think

          {begin,commit,rollback}

          _transaction are too long... and I would use begin(), commit() and rollback() personally.

          Show
          Penny Leach added a comment - i also mildly think {begin,commit,rollback} _transaction are too long... and I would use begin(), commit() and rollback() personally.
          Hide
          Petr Škoda added a comment -

          I think that in majority of cases you would just use one catch block that rolls back and rethrows the exception.

          Show
          Petr Škoda added a comment - I think that in majority of cases you would just use one catch block that rolls back and rethrows the exception.
          Hide
          Tim Hunt added a comment -

          Thank you for thinking about this Eloy.

          On naming, I propose

          $DB->start_transaction();
          $DB->finish_transaction();
          $DB->abort_transaction();

          I still don't understand why people object to the token bit, especially as I have already implemented it, and it makes the link between matching start and end transaction clearer, but I also don't care if you throw that away.

          Except that, using a token object that will automatically be __destructed might mean that you could avoid having to write the

          } catch (dml_exception $e) {
              $DB->rollback_transaction();
              throw $e
          } catch (ddl_exception $e) { // Just in case we want to play with transactions at install/upgrade
              $DB->rollback_transaction();
              throw $e
          

          bit in a million different places, which would be nice.

          Hmm. At the very least can we make a common superclass of dml_exception and ddl_exception, to save one catch clause.

          Show
          Tim Hunt added a comment - Thank you for thinking about this Eloy. On naming, I propose $DB->start_transaction(); $DB->finish_transaction(); $DB->abort_transaction(); I still don't understand why people object to the token bit, especially as I have already implemented it, and it makes the link between matching start and end transaction clearer, but I also don't care if you throw that away. Except that, using a token object that will automatically be __destructed might mean that you could avoid having to write the } catch (dml_exception $e) { $DB->rollback_transaction(); throw $e } catch (ddl_exception $e) { // Just in case we want to play with transactions at install/upgrade $DB->rollback_transaction(); throw $e bit in a million different places, which would be nice. Hmm. At the very least can we make a common superclass of dml_exception and ddl_exception, to save one catch clause.
          Hide
          Petr Škoda added a comment - - edited

          oh, why would you want to differentiate between different exceptions? you have to rollback in any case, right?

          We could also take the exception as mandatory parameter in rollback method:

          try {
              $DB->begin_transaction();
              // Execute a bunch of SQLs here
              // trying to keep out other things like files and friends as possible
              $DB->commit_transaction();
          } catch (Exception $e) {
              $DB->rollback_transaction($e);
          }
          

          grrrrr

          Show
          Petr Škoda added a comment - - edited oh, why would you want to differentiate between different exceptions? you have to rollback in any case, right? We could also take the exception as mandatory parameter in rollback method: try { $DB->begin_transaction(); // Execute a bunch of SQLs here // trying to keep out other things like files and friends as possible $DB->commit_transaction(); } catch (Exception $e) { $DB->rollback_transaction($e); } grrrrr
          Hide
          Sam Marshall added a comment - - edited

          I agree with Penny, it's a big pain even compared to my transaction_wrapper for instance (and I think it could be better than that).

          Do we even need rollback feature at all? I know there was some discussion about this. But since rollback is not possible for people without transaction support. IMO, fixing the database (if required) should be the responsibility of whoever catches an exception (and it can be any exception, not just a db one).

          My proposal would be something like,

          For ordinary code:

          $token = $DB->start_transaction();
          
          // ...
          
          $DB->finish_transaction($token);
          

          (token is kind of optional, I like it if these things are checked though as it is more likely to detect errors at right point of code).

          In the unusual case that it is necessary to catch an exception and handle it (ie not leave the database in the broken transaction):

          try {
            // ...
          } catch(Exception $e) {
            // Handle it, and...
          
            $db->clear_errors(); // Will do rollback if necessary
          }
          

          For most cases where exceptions are only displayed as errors, it should be correct and OK to use the automatic rollback facility (that applies for any transaction which is not committed)

          Also, to assist developers, code should enforce this. Consider this situation:

          try {
            // Database code throws a dml/ddl exception
          } catch(Exception $e) {
            // oops I didn't clear errors
          }
          
          // Now I do more database code, which will work in mysql!
          

          This code will 'work' in MySQL with the broken table model, but not when inside a transaction on a db that supports transactions. As a solution, I propose that whenever a dml/ddl exception is thrown, the database should automatically go into 'there are errors' state. In that state, any further call to a database function except clear_errors automatically throws exception straight away, until you call clear_errors.

          Anyhow maybe there was something in the previous discussion which made this approach impossible, but if not, it certainly seems a lot nicer for coders than eloy's source above

          eta: Hah! I didn't look at tim's design, apparently that had token in too? neat.

          Show
          Sam Marshall added a comment - - edited I agree with Penny, it's a big pain even compared to my transaction_wrapper for instance (and I think it could be better than that). Do we even need rollback feature at all? I know there was some discussion about this. But since rollback is not possible for people without transaction support. IMO, fixing the database (if required) should be the responsibility of whoever catches an exception (and it can be any exception, not just a db one). My proposal would be something like, For ordinary code: $token = $DB->start_transaction(); // ... $DB->finish_transaction($token); (token is kind of optional, I like it if these things are checked though as it is more likely to detect errors at right point of code). In the unusual case that it is necessary to catch an exception and handle it (ie not leave the database in the broken transaction): try { // ... } catch (Exception $e) { // Handle it, and... $db->clear_errors(); // Will do rollback if necessary } For most cases where exceptions are only displayed as errors, it should be correct and OK to use the automatic rollback facility (that applies for any transaction which is not committed) Also, to assist developers, code should enforce this. Consider this situation: try { // Database code throws a dml/ddl exception } catch (Exception $e) { // oops I didn't clear errors } // Now I do more database code, which will work in mysql! This code will 'work' in MySQL with the broken table model, but not when inside a transaction on a db that supports transactions. As a solution, I propose that whenever a dml/ddl exception is thrown, the database should automatically go into 'there are errors' state. In that state, any further call to a database function except clear_errors automatically throws exception straight away, until you call clear_errors. Anyhow maybe there was something in the previous discussion which made this approach impossible, but if not, it certainly seems a lot nicer for coders than eloy's source above eta: Hah! I didn't look at tim's design, apparently that had token in too? neat.
          Hide
          Petr Škoda added a comment -

          I am afraid developers will always find ways to abuse transactions and exceptions, I think we can easily review all such places using simple grep search in core code once a month or two. Tuesday reviews are another good opportunity to inform devs about wrong coding style.

          The simple use without try catch and rollback is also possible, we do not need tokens for that - any exception propagates to the default exception handler which rollbacks the transaction if active.

          Show
          Petr Škoda added a comment - I am afraid developers will always find ways to abuse transactions and exceptions, I think we can easily review all such places using simple grep search in core code once a month or two. Tuesday reviews are another good opportunity to inform devs about wrong coding style. The simple use without try catch and rollback is also possible, we do not need tokens for that - any exception propagates to the default exception handler which rollbacks the transaction if active.
          Hide
          Martín Langhoff added a comment -

          Tim's patch is recommended reading, sp the 'token' bit

          Petr said:
          > any exception propagates to the default exception handler

          that is just not true. Any exception handler that gets declared can receive and "handle" the exception. And it is not obvious – definitely not easily greppable – how our callstack will end up (for an example, see the wild things print_error() ends up calling in HEAD these days).

          So we have to be insanely careful with this.

          To be frank, all the complexity we can see here, and the complexity we'll allow to be embedded in the code with no visible indicators makes me side with Eloy, and say first stage: nested transactions should not work, and an exception thrown.

          If we come across something that absolutely needs nested transactions, we can come back to this discussion and – with the benefit of having a specific case – see what' s the best approach.

          Show
          Martín Langhoff added a comment - Tim's patch is recommended reading, sp the 'token' bit Petr said: > any exception propagates to the default exception handler that is just not true. Any exception handler that gets declared can receive and "handle" the exception. And it is not obvious – definitely not easily greppable – how our callstack will end up (for an example, see the wild things print_error() ends up calling in HEAD these days). So we have to be insanely careful with this. To be frank, all the complexity we can see here, and the complexity we'll allow to be embedded in the code with no visible indicators makes me side with Eloy, and say first stage: nested transactions should not work, and an exception thrown. If we come across something that absolutely needs nested transactions, we can come back to this discussion and – with the benefit of having a specific case – see what' s the best approach.
          Hide
          Petr Škoda added a comment - - edited

          >> any exception propagates to the default exception handler
          >that is just not true.

          No, it is true. I wrote the exception handler we have now, it is not finished yet, I hoped it would be done together with the pagelib rewrite Tim did recently.

          Mandatory steps in each exception handler are:
          1/ rollback transactions and log this problem - this should be implemented in the $DB itself
          2/ log exception - again should be a system function
          3/ print some info - this is where each handler differs
          3a) print simple error - before the OUTPUT is fully initialised
          3b) print standard error using OUTPUT
          3c) use zend xmlrpc server class to send XML error back
          etc.

          If you want more information in the ex handler the most important part is where was the first level of transaction started and the type of exception that caused the problem, again you do not need tokens for this.

          I already found one part that absolutely needs the transactions - the external API functions that modify multiple records. There is no standards compliant way to return incomplete results such as 1 of 3 records successfulyl modified, so instead at present we roll back and report failure. The problem is we can not use exceptions anywhere else in core code, but also we can not use exceptions in plugins like enrol or auth because they are supposed to use the external API

          Petr

          Show
          Petr Škoda added a comment - - edited >> any exception propagates to the default exception handler >that is just not true. No, it is true. I wrote the exception handler we have now, it is not finished yet, I hoped it would be done together with the pagelib rewrite Tim did recently. Mandatory steps in each exception handler are: 1/ rollback transactions and log this problem - this should be implemented in the $DB itself 2/ log exception - again should be a system function 3/ print some info - this is where each handler differs 3a) print simple error - before the OUTPUT is fully initialised 3b) print standard error using OUTPUT 3c) use zend xmlrpc server class to send XML error back etc. If you want more information in the ex handler the most important part is where was the first level of transaction started and the type of exception that caused the problem, again you do not need tokens for this. I already found one part that absolutely needs the transactions - the external API functions that modify multiple records. There is no standards compliant way to return incomplete results such as 1 of 3 records successfulyl modified, so instead at present we roll back and report failure. The problem is we can not use exceptions anywhere else in core code, but also we can not use exceptions in plugins like enrol or auth because they are supposed to use the external API Petr
          Hide
          Martín Langhoff added a comment - - edited

          >>> any exception propagates to the default exception handler
          >> that is just not true.
          > No, it is true.

          Maybe I haven't been clear – "default exception handler" means anyone can override it:

               try {
                    foo(); // which will do many complex things
                              // including setting up a transaction
                              // and exec'ing  SQL that fails
                              // but the caller is naive so...
               } catch ($e) {
                   // yeah, foo fails sometimes, log it
                   // and keep running...
                  error_log("foo() barfed again, sigh")
               } 
          

          as you see, there is no 'throw($e)' in there.

          > I already found one part that absolutely needs the transactions

          Definitely. But I am saying nested transactions. Does your code absolutely need nested transactions?

          Can we defer implementing them (and put instead code that generates a warning), given that they are a recipe for disaster? OR implementing them on a

               if (defined('DB_NESTED_TRANSACTIONS_YES_I_WANT_TO_SUFFER')) {
          

          for the adventurous?

          Show
          Martín Langhoff added a comment - - edited >>> any exception propagates to the default exception handler >> that is just not true. > No, it is true. Maybe I haven't been clear – "default exception handler" means anyone can override it: try { foo(); // which will do many complex things // including setting up a transaction // and exec'ing SQL that fails // but the caller is naive so... } catch ($e) { // yeah, foo fails sometimes, log it // and keep running... error_log( "foo() barfed again, sigh" ) } as you see, there is no 'throw($e)' in there. > I already found one part that absolutely needs the transactions Definitely. But I am saying nested transactions. Does your code absolutely need nested transactions? Can we defer implementing them (and put instead code that generates a warning), given that they are a recipe for disaster? OR implementing them on a if (defined('DB_NESTED_TRANSACTIONS_YES_I_WANT_TO_SUFFER')) { for the adventurous?
          Hide
          Petr Škoda added a comment - - edited

          hmm, yes exceptions are very easy to abuse sorry, for the misunderstanding
          If anybody blindly catches exceptions and ignores the problems the code will definitely fail and db would be borked.

          We did not use transactions yet much, so no we do not need nested exceptions, but the fact that external API is going to use them actually prevents anybody else from using them in majority of cases.

          Show
          Petr Škoda added a comment - - edited hmm, yes exceptions are very easy to abuse sorry, for the misunderstanding If anybody blindly catches exceptions and ignores the problems the code will definitely fail and db would be borked. We did not use transactions yet much, so no we do not need nested exceptions, but the fact that external API is going to use them actually prevents anybody else from using them in majority of cases.
          Hide
          Penny Leach added a comment -

          PS I like Sam's idea that once an exception has been thrown, $DB object refuses to continue executing queries. This helps with Mysql's ... specialness.

          Also since Tim has already written tokens, I don't see what the problem is keeping them - it helps debug errors much easier

          (eg you tried to close a transaction called pony, while you still had an open horse transaction) - helps us see the nesting errors.

          Show
          Penny Leach added a comment - PS I like Sam's idea that once an exception has been thrown, $DB object refuses to continue executing queries. This helps with Mysql's ... specialness. Also since Tim has already written tokens, I don't see what the problem is keeping them - it helps debug errors much easier (eg you tried to close a transaction called pony, while you still had an open horse transaction) - helps us see the nesting errors.
          Hide
          Petr Škoda added a comment -

          if we use the tokens we can not use the simple case without try catch because the inner transactions will not be closed and it would be printing debug warnings when any exception thrown

          I do not like the idea that we should somehow cripple DML when rollbacking, because we use DML also for DB sessions - it would undo all changes done to sessions before start of the transaction

          the more I think about this the more problems I see:

          • external events are not rolled back (solvable, see linked bug)
          • session is not rolled back
          • old style file operations are not rolled back
          • internal caches are not rolled back
          • shared memory does not roll back (not used anywhere in 2.0 yet)

          I was reviewing the file API a bit too, the DB rollback should work fine there (includes file deletes) - the only problem might be orphaned file contents in file pool.

          I think it should be mandatory to send exception as parameter when rolling back, because otherwise we will not have enough information when logging rollback problems

          hmm, you are not supposed to be nesting transactions every day and if you do it should be relatively simple code, if not you are most probably doing something wrong

          > Also since Tim has already written tokens
          the code is not finished and can not be used as is - for example the rollback in token destruct is going to break DB sessions, default exception handler is not updated, the simple case without try catch does not work when nesting (all of these problems are solvable of course)

          the fact that some patch exists is not an argument for me, we will have to live with the decision here for many years I am afraid - I would prefer to "waste" several days now in order to make good decision

          To sum it up, Tim is proposing:

          // option A
          $transaction = $DB->start_transaction('optional_imlazy');
          try {
            // Do stuff ...
            $transaction->commit();
          } catch (Exception $ex) {
            $transaction->rollback($ex);
          }
          

          I was proposing more traditional approach:

          // option B
          $DB->start_transaction();
          try {
            // Do stuff ...
            $DB->commit_transaction();
          } catch (Exception $ex) {
            $DB->rollback_transaction($ex);
          }
          
          // or simply expecting rollback is handled by default exception handler or another transaction layer
          $DB->start_transaction();
          // Do stuff ...
          $DB->commit_transaction();
          

          Both of these options support nesting, both of them can work in one level only, option A can give more debugging options (this benefit is lost if we allow simple syntax without rollback), option B is more backwards compatible

          Show
          Petr Škoda added a comment - if we use the tokens we can not use the simple case without try catch because the inner transactions will not be closed and it would be printing debug warnings when any exception thrown I do not like the idea that we should somehow cripple DML when rollbacking, because we use DML also for DB sessions - it would undo all changes done to sessions before start of the transaction the more I think about this the more problems I see: external events are not rolled back (solvable, see linked bug) session is not rolled back old style file operations are not rolled back internal caches are not rolled back shared memory does not roll back (not used anywhere in 2.0 yet) I was reviewing the file API a bit too, the DB rollback should work fine there (includes file deletes) - the only problem might be orphaned file contents in file pool. I think it should be mandatory to send exception as parameter when rolling back, because otherwise we will not have enough information when logging rollback problems hmm, you are not supposed to be nesting transactions every day and if you do it should be relatively simple code, if not you are most probably doing something wrong > Also since Tim has already written tokens the code is not finished and can not be used as is - for example the rollback in token destruct is going to break DB sessions, default exception handler is not updated, the simple case without try catch does not work when nesting (all of these problems are solvable of course) the fact that some patch exists is not an argument for me, we will have to live with the decision here for many years I am afraid - I would prefer to "waste" several days now in order to make good decision To sum it up, Tim is proposing: // option A $transaction = $DB->start_transaction('optional_imlazy'); try { // Do stuff ... $transaction->commit(); } catch (Exception $ex) { $transaction->rollback($ex); } I was proposing more traditional approach: // option B $DB->start_transaction(); try { // Do stuff ... $DB->commit_transaction(); } catch (Exception $ex) { $DB->rollback_transaction($ex); } // or simply expecting rollback is handled by default exception handler or another transaction layer $DB->start_transaction(); // Do stuff ... $DB->commit_transaction(); Both of these options support nesting, both of them can work in one level only, option A can give more debugging options (this benefit is lost if we allow simple syntax without rollback), option B is more backwards compatible
          Hide
          Petr Škoda added a comment -

          In any case this discussion is great - now most core developers perfectly understand transactions and we touched the topic of exceptions too, yay!!

          thanks, Moodle community rocks :-D

          Show
          Petr Škoda added a comment - In any case this discussion is great - now most core developers perfectly understand transactions and we touched the topic of exceptions too, yay!! thanks, Moodle community rocks :-D
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, thanks for input. Some comments:

          • About names: I like Tim's proposal ($DB->[start|finish|abort]_transaction()) mainly because it leaves the "false" commit/rollback out from the naming. About shorter names, I think we need the "transaction" word there, being lazy we could use "trans" too, lol.
          • About dml/ddl/others exceptions: I wrote the ddl/others catch blocks there only to show them (not to say that they are needed always at all). ddl exceptions only need to be caught (if we really want to do so) in install and upgrade code. And others only if there are some other code in the try {} block needing it. Ideally all our "in-transaction" code will be, always, 100% dml code (at least the inner bits), so we only need that exception there in 99% of cases. How "hybrid" code (that having both dml and other operations, also throwing exceptions) like adding one file to the pool for example, needs to be discussed if we want to use transactions for it (although I think we can be consistent without them).
          • About the "abused" use of default catching, as commented by Martin, well, I think it's, in any case, a matter of educate developers, exactly the same that the progress of moving from get_records() to get_recordset() and others. We can document, execute all sort of scripts looking for bad uses. For example we can look for any default/moodle_exception block and report it should be using another (more detailed exception). Or we can look for try-catch blocks using transactions and not having the dml exception being caught.

          In any case, I don't see why your example, Martin, is problematic at all (sure I'm missing something):

          1) The foo() contains the top-level transaction usage. So, if it fails, it will mark (and really rollback DB, as it's level 1) the transaction as failed and throw dml exception. Then your "default catch handler" will error_log(). No problem.
          2) The foo() isn't the top-level transaction. So, if it fails, it will mark (but not really rollback the DB) the transaction, then your "default catch handler" will error_log() and, finally the top-level transaction will try to commit, but that won't happen because the transactions has been marked as "rollback-able". So one "transaction exception" will be thrown and the reall rollback will happen. No problem either.

          • About the "token" the point I more like it is because of the "autodetection" (on destruction) of the object not being finished/aborted immediately, while the "no-token" alternative delays that to then end of the script (hopefully somewhere before disposing the database, so we'll have access to all the debugging/output handling). The other utility of the token (to pair start/finish calls doesn't seem really important for me). But I don't buy the idea of getting objects out from $DB at all, although, well, sincerely, we are already doing that for recordsets I think I can be flexible at this point if everybody else agrees.
          • About the any dml/ddl exception putting the $DB into "exception has happened" state, uhm... interesting idea... although the "default exception handler" will need to clear that state, just in case it needs to log to DB or so. And catch blocks should be able to do so too, agree, as suggested by Sam.

          Also, if we want to avoid the try/catch blocks for normal usage then we must guarantee that any dml/ddl exception sets the transaction status (if in-transaction only, obviously) to "rollback-able" so, once again, the default exception handler, in charge of closing all the levels will perform the final (and real) rollback, or alternatively, the outer in-transaction finish will, if for any cause somebody has broken the exception chain.

          In any case, remember that only one (dml) exception must be caught in my alternative. And I find it more "explicit". IMO a small pay for people wanting to use those non-existent "nested transactions". Perhaps your alternative leaves too much relying in the magic dark of the default exception handler, when it should do that magic, agree, but only as an exceptional measure (effectively roll-backing but throwing one "transaction exception"). Normal behaviour should be to have things finished/aborted in top level of the transaction and not in the default exception handler.

          Ciao

          Please, round 1 iteration 2 comments now, after that, I'll remake the original points to start round 2 (hopefully final). Thanks everybody.

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, thanks for input. Some comments: About names: I like Tim's proposal ($DB-> [start|finish|abort] _transaction()) mainly because it leaves the "false" commit/rollback out from the naming. About shorter names, I think we need the "transaction" word there, being lazy we could use "trans" too, lol. About dml/ddl/others exceptions: I wrote the ddl/others catch blocks there only to show them (not to say that they are needed always at all). ddl exceptions only need to be caught (if we really want to do so) in install and upgrade code. And others only if there are some other code in the try {} block needing it. Ideally all our "in-transaction" code will be, always, 100% dml code (at least the inner bits), so we only need that exception there in 99% of cases. How "hybrid" code (that having both dml and other operations, also throwing exceptions) like adding one file to the pool for example, needs to be discussed if we want to use transactions for it (although I think we can be consistent without them). About the "abused" use of default catching, as commented by Martin, well, I think it's, in any case, a matter of educate developers, exactly the same that the progress of moving from get_records() to get_recordset() and others. We can document, execute all sort of scripts looking for bad uses. For example we can look for any default/moodle_exception block and report it should be using another (more detailed exception). Or we can look for try-catch blocks using transactions and not having the dml exception being caught. In any case, I don't see why your example, Martin, is problematic at all (sure I'm missing something): 1) The foo() contains the top-level transaction usage. So, if it fails, it will mark (and really rollback DB, as it's level 1) the transaction as failed and throw dml exception. Then your "default catch handler" will error_log(). No problem. 2) The foo() isn't the top-level transaction. So, if it fails, it will mark (but not really rollback the DB) the transaction, then your "default catch handler" will error_log() and, finally the top-level transaction will try to commit, but that won't happen because the transactions has been marked as "rollback-able". So one "transaction exception" will be thrown and the reall rollback will happen. No problem either. About the "token" the point I more like it is because of the "autodetection" (on destruction) of the object not being finished/aborted immediately, while the "no-token" alternative delays that to then end of the script (hopefully somewhere before disposing the database, so we'll have access to all the debugging/output handling). The other utility of the token (to pair start/finish calls doesn't seem really important for me). But I don't buy the idea of getting objects out from $DB at all, although, well, sincerely, we are already doing that for recordsets I think I can be flexible at this point if everybody else agrees. About the any dml/ddl exception putting the $DB into "exception has happened" state, uhm... interesting idea... although the "default exception handler" will need to clear that state, just in case it needs to log to DB or so. And catch blocks should be able to do so too, agree, as suggested by Sam. Also, if we want to avoid the try/catch blocks for normal usage then we must guarantee that any dml/ddl exception sets the transaction status (if in-transaction only, obviously) to "rollback-able" so, once again, the default exception handler, in charge of closing all the levels will perform the final (and real) rollback, or alternatively, the outer in-transaction finish will, if for any cause somebody has broken the exception chain. In any case, remember that only one (dml) exception must be caught in my alternative. And I find it more "explicit". IMO a small pay for people wanting to use those non-existent "nested transactions". Perhaps your alternative leaves too much relying in the magic dark of the default exception handler, when it should do that magic, agree, but only as an exceptional measure (effectively roll-backing but throwing one "transaction exception"). Normal behaviour should be to have things finished/aborted in top level of the transaction and not in the default exception handler. Ciao Please, round 1 iteration 2 comments now, after that, I'll remake the original points to start round 2 (hopefully final). Thanks everybody.
          Hide
          Dan Poltawski added a comment -

          I really like Sams proposal to fail $DB stuff until clear_errors() is called.

          But I am sure there are going to be code paths with crap data which we are unlikely to test - are the side effects of failing all future database interactions at that stage going to be more catastrophic than allowing things to continue? Perhaps it would make sense to only enable this in developer debug mode?

          Show
          Dan Poltawski added a comment - I really like Sams proposal to fail $DB stuff until clear_errors() is called. But I am sure there are going to be code paths with crap data which we are unlikely to test - are the side effects of failing all future database interactions at that stage going to be more catastrophic than allowing things to continue? Perhaps it would make sense to only enable this in developer debug mode?
          Hide
          Martin Dougiamas added a comment -

          Great to see this discussion!

          Show
          Martin Dougiamas added a comment - Great to see this discussion!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi,

          just attaching one simple diagram showing all the chaining levels we have when handling exceptions and transactions at the same time.

          I've tried to explain the (my) ideal behaviour on each level thinking how the system will work under different situations (use cases).

          Feel free to comment /add use cases where something could fail badly. Please use the "nomenclature" above and explain the flow leading to problems.

          Hope it helps, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, just attaching one simple diagram showing all the chaining levels we have when handling exceptions and transactions at the same time. I've tried to explain the (my) ideal behaviour on each level thinking how the system will work under different situations (use cases). Feel free to comment /add use cases where something could fail badly. Please use the "nomenclature" above and explain the flow leading to problems. Hope it helps, ciao
          Hide
          Petr Škoda added a comment - - edited

          looks like you ahve switched the default exception handler and footer code, any uncaught exception skips normal footer - there is that ugly hack that tries to close all containers if header already printed and prints another footer and stops.

          Hmmm, the open transactions have to be detected in:
          1/ default exception handlers - force rollback
          2/ normal print footer (because we can not print warnings after that) - just detect, do not rollback in case there is something done after the print footer which is usually not expected
          3/ right before committing session changes to DB - the session is not affected by rollback, hmm in future we coudl rollback session too if we make a snapshot when starting transaction
          4/ some shutdown function just in case it somehow gets through till there - force rollback

          Show
          Petr Škoda added a comment - - edited looks like you ahve switched the default exception handler and footer code, any uncaught exception skips normal footer - there is that ugly hack that tries to close all containers if header already printed and prints another footer and stops. Hmmm, the open transactions have to be detected in: 1/ default exception handlers - force rollback 2/ normal print footer (because we can not print warnings after that) - just detect, do not rollback in case there is something done after the print footer which is usually not expected 3/ right before committing session changes to DB - the session is not affected by rollback, hmm in future we coudl rollback session too if we make a snapshot when starting transaction 4/ some shutdown function just in case it somehow gets through till there - force rollback
          Hide
          Petr Škoda added a comment -

          we have discussed this a bit more wit h Eloy, we both understand it the same way, it is just me who did not understand the picture much

          Show
          Petr Škoda added a comment - we have discussed this a bit more wit h Eloy, we both understand it the same way, it is just me who did not understand the picture much
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, yes. Not all the "boxes" above are always involved.

          1. default exception handler will catch uncaught dml/ddl/rollback exceptions and will perform the rollback() and report exception honouring debug/output settings.
          2. footer will "detect" wrong situations ( in-transaction status) and will throw rollback_exception if so. That rollback_exception with be caught by default exception handler (little down-arrow in the diagram) and we'll be in situation 1.
          3. if neither default exception hadler (1) nor footer (2) has been able to detect the abnormal situation (because they don't exist or whatever), then there is one last chance to check for in-transaction status in the moodle_request_shutdown function. We still can honour debug/output settings (although out from the <html>). This won't throw any exception nor rollback at all, just debug/output.
          4. Finally the DB object, when being disposed, will look once more for in-transaction status, will log error (without honouring debug/output settings, because we aren't in request at that level) and will rollback() / clean up things. And that will happen before writing session stuff to DB, agree.

          So, normal (correct) operation will involve using only green/yellow areas (if everything is properly try-catch on each transaction).

          And then, if anything goes wrong, "footer" will detect it (for requests having "footer", obviously) and will delegate the process (back) to the default handler. Still within <html>.

          And finally shutdown will detect it for the rest of situations, producing debugging and delegating the task of rollback() to connection dispose. Useful for requests not being <html> based that haven't being processed by the default exception handler.

          Pretty covered IMO. Just trying to imagine situations were the diagram above wouldn't work.

          So I think we are agreeing Petr (yay!). Just the graph isn't as self-explanatory as I had imagined, lol. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, yes. Not all the "boxes" above are always involved. default exception handler will catch uncaught dml/ddl/rollback exceptions and will perform the rollback() and report exception honouring debug/output settings. footer will "detect" wrong situations ( in-transaction status) and will throw rollback_exception if so. That rollback_exception with be caught by default exception handler (little down-arrow in the diagram) and we'll be in situation 1. if neither default exception hadler (1) nor footer (2) has been able to detect the abnormal situation (because they don't exist or whatever), then there is one last chance to check for in-transaction status in the moodle_request_shutdown function. We still can honour debug/output settings (although out from the <html>). This won't throw any exception nor rollback at all, just debug/output. Finally the DB object, when being disposed, will look once more for in-transaction status, will log error (without honouring debug/output settings, because we aren't in request at that level) and will rollback() / clean up things. And that will happen before writing session stuff to DB, agree. So, normal (correct) operation will involve using only green/yellow areas (if everything is properly try-catch on each transaction). And then, if anything goes wrong, "footer" will detect it (for requests having "footer", obviously) and will delegate the process (back) to the default handler. Still within <html>. And finally shutdown will detect it for the rest of situations, producing debugging and delegating the task of rollback() to connection dispose. Useful for requests not being <html> based that haven't being processed by the default exception handler. Pretty covered IMO. Just trying to imagine situations were the diagram above wouldn't work. So I think we are agreeing Petr (yay!). Just the graph isn't as self-explanatory as I had imagined, lol. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki here there are the latest conclusions for the "nested transactions" thing, trying to join as many ideas as possible from previous discussions:

          General principles:

          1. Everybody thinks it's a good idea to have those logical nested transactions in Moodle.
          2. Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information.
          3. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback.
          4. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback.
          5. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level.
          6. It will be optional to catch dml_exceptions when using transactions, but if they are caught, then it's mandatory to:
            1. mark the transaction for rollback()
            2. re-throw the dml exception
          7. API will work in a natural and similar fashion to current recordsets one.

          The API:

          1. One object: moodle_nested_transaction will handle the whole thing (for each level)
          2. $DB will be the responsible to instantiate / accumulate / pair moodle_nested_transactions
          3. Each moodle_nested_transaction will be able to set it's one finished status (mark for commit / mark rollback)
          4. Any moodle_nested_transaction, once out from its scope if unfinished will throw transaction_exception
          5. Normal usage of the moodle_nested_transaction will be:
            $transaction = $DB->start_transaction($optional_name = null);
            // Perform some $DB stuff
            $transaction->commit();
            

            QUESTION: any error will end with one transaction_exception and one dml_exception? Not 100% sure here.

          6. If, for any reason, developer needs to catch dml_exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way:
            try {
                $transaction = $DB->start_transaction($optional_name = null);
                // Perform some $DB stuff
                $transaction->commit();
            } catch (dml_exception $e) {
                $transaction->rollback();
                throw $e;
            }
            

            Note: the original dml_exception will be maintained.

          The Flow:

          1. Any default exception handler will:
            1. Catch uncaught transaction_exception exceptions,
            2. Properly perform the DB rollback
            3. debug/error/log honouring related settings.
            4. inform with as many details as possible (token, place... whatever).
          2. Any "footer" (meaning some place before ending <html> output will:
            1. Detect "in-transaction" status.
            2. Throw transaction_exception (that will be caught, once again, by the default exception handler)
          3. moodle_request_shutdown will:
            1. Detect "in-transaction" status.
            2. debug/error/log honouring related settings (out from <html>!)
          4. $DB->dispose will:
            1. Detect "in-transaction" status.
            2. Properly perform the DB rollback
            3. log error (not possible to honour settings!)
            4. inform with as many details as possible (token, place... whatever).

          And that's all. Please, comment, specially the QUESTION above if we are using the "short" coding alternative. Or anything else you want to add/object/array :-D

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki here there are the latest conclusions for the "nested transactions" thing, trying to join as many ideas as possible from previous discussions: General principles: Everybody thinks it's a good idea to have those logical nested transactions in Moodle. Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level. It will be optional to catch dml_exceptions when using transactions, but if they are caught, then it's mandatory to: mark the transaction for rollback() re-throw the dml exception API will work in a natural and similar fashion to current recordsets one. The API: One object: moodle_nested_transaction will handle the whole thing (for each level) $DB will be the responsible to instantiate / accumulate / pair moodle_nested_transactions Each moodle_nested_transaction will be able to set it's one finished status (mark for commit / mark rollback) Any moodle_nested_transaction, once out from its scope if unfinished will throw transaction_exception Normal usage of the moodle_nested_transaction will be: $transaction = $DB->start_transaction($optional_name = null ); // Perform some $DB stuff $transaction->commit(); QUESTION: any error will end with one transaction_exception and one dml_exception? Not 100% sure here. If, for any reason, developer needs to catch dml_exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way: try { $transaction = $DB->start_transaction($optional_name = null ); // Perform some $DB stuff $transaction->commit(); } catch (dml_exception $e) { $transaction->rollback(); throw $e; } Note: the original dml_exception will be maintained. The Flow: Any default exception handler will: Catch uncaught transaction_exception exceptions, Properly perform the DB rollback debug/error/log honouring related settings. inform with as many details as possible (token, place... whatever). Any "footer" (meaning some place before ending <html> output will: Detect "in-transaction" status. Throw transaction_exception (that will be caught, once again, by the default exception handler) moodle_request_shutdown will: Detect "in-transaction" status. debug/error/log honouring related settings (out from <html>!) $DB->dispose will: Detect "in-transaction" status. Properly perform the DB rollback log error (not possible to honour settings!) inform with as many details as possible (token, place... whatever). And that's all. Please, comment, specially the QUESTION above if we are using the "short" coding alternative. Or anything else you want to add/object/array :-D Ciao
          Hide
          Petr Škoda added a comment -

          oh, Eloy we need to know the exception when rolling back, we can just

          $tr->abort_transaction($ex);
          

          which would automatically rethrow the exception

          Show
          Petr Škoda added a comment - oh, Eloy we need to know the exception when rolling back, we can just $tr->abort_transaction($ex); which would automatically rethrow the exception
          Hide
          Petr Škoda added a comment -

          the example is not correct because in majority of cases we are going to catch (Exception $e) iho

          Show
          Petr Škoda added a comment - the example is not correct because in majority of cases we are going to catch (Exception $e) iho
          Hide
          Martín Langhoff added a comment -

          Two comments

          1 - We don't all agree it is a good idea to have nested transactions. I vote yes for transactions, not to nesting unless we can get a good example. Eloy, you started saying we should keep them separate – have you got a good case where you really need nested tx?

          2 - You correctly state that "If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level." A really nasty result of this is that your code will say $db->commit() but you have no idea whether the commit actually succeeds.

          This #2 is rather problematic if you need to change any state that is not in our DB. And I think it means it is a misdesign to do nested transactions.

          If we do not do nested transactions, we can say: on DBs that support transactions, $db->commit() means COMMIT. This is an important feature when sync'ing with external systems for example.

          COMMIT guarantees it is done, for real. If we go the nested way, it will never mean anything, even on DBs that are ACID.

          For example: with nested tx, how can you delete a file?

            $db->begin()
            $db->execute( DELETE whatever row has the ref to the file )
            $db->commit()
            if ($db->count (references to that file) === 0) {
               unlink(/path/to/file)
            }
            return true;
          

          In the above example, if the calling code has an unrelated error and issues a rollback, we've corrupted our state.

          Show
          Martín Langhoff added a comment - Two comments 1 - We don't all agree it is a good idea to have nested transactions. I vote yes for transactions, not to nesting unless we can get a good example. Eloy, you started saying we should keep them separate – have you got a good case where you really need nested tx? 2 - You correctly state that "If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level." A really nasty result of this is that your code will say $db->commit() but you have no idea whether the commit actually succeeds. This #2 is rather problematic if you need to change any state that is not in our DB. And I think it means it is a misdesign to do nested transactions. If we do not do nested transactions, we can say: on DBs that support transactions, $db->commit() means COMMIT. This is an important feature when sync'ing with external systems for example. COMMIT guarantees it is done, for real. If we go the nested way, it will never mean anything, even on DBs that are ACID. For example: with nested tx, how can you delete a file? $db->begin() $db->execute( DELETE whatever row has the ref to the file ) $db->commit() if ($db->count (references to that file) === 0) { unlink(/path/to/file) } return true ; In the above example, if the calling code has an unrelated error and issues a rollback, we've corrupted our state.
          Hide
          Petr Škoda added a comment -

          you sure can handle files now with nested DB transactions - because our files line in the files table now

          Show
          Petr Škoda added a comment - you sure can handle files now with nested DB transactions - because our files line in the files table now
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Comments:

          General:

          1) First of all, agree with ML (as previously said). Nested transactions are a fallacy.
          2) I continue thinking that 1-level transactions are ok (in the request.php scripts) and do whatever ppl expect to do (real commits and real rollbacks). But at the same time, the bigger is the transaction, the more probable it will contain non-recoverable situations, like the "delete a file" example above.
          3) As it seems that developers love (and are using with success, OU, Mahara) them, I'm trying to raise some sort of agreement about them for Moodle.
          4) Not being able to rely on them (because of MySQL ISAM) is a bloody hell, making them simple useful to reduce (not eliminate) the potential damage caused by any DB error. Far from their real utility. Only if we start requiring InnoDB they will be really useful (being able to rely on them).

          Comments:

          1) Changed API names as requested (knowing that they don't really perform the commit/rollback)
          2) Agree about passing original exception to $transaction->rollback($ex)
          3) I still don't get my QUESTION above. In case of being using the the reduced syntax, which exception will be thrown, the original one cause by the code or the transaction_exception one caused by the unpaired unfinished nested_transaction?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Comments: General: 1) First of all, agree with ML (as previously said). Nested transactions are a fallacy. 2) I continue thinking that 1-level transactions are ok (in the request.php scripts) and do whatever ppl expect to do (real commits and real rollbacks). But at the same time, the bigger is the transaction, the more probable it will contain non-recoverable situations, like the "delete a file" example above. 3) As it seems that developers love (and are using with success, OU, Mahara) them, I'm trying to raise some sort of agreement about them for Moodle. 4) Not being able to rely on them (because of MySQL ISAM) is a bloody hell, making them simple useful to reduce (not eliminate) the potential damage caused by any DB error. Far from their real utility. Only if we start requiring InnoDB they will be really useful (being able to rely on them). Comments: 1) Changed API names as requested (knowing that they don't really perform the commit/rollback) 2) Agree about passing original exception to $transaction->rollback($ex) 3) I still don't get my QUESTION above. In case of being using the the reduced syntax, which exception will be thrown, the original one cause by the code or the transaction_exception one caused by the unpaired unfinished nested_transaction? Ciao
          Hide
          Martín Langhoff added a comment -

          Petr, this is a problem specifically with the Files implementation. Do read my (pseudocode) example carefully, you will see the problem.

          In general, our move to transactions should get us in the right path towards (in some far future) using transactions properly, and requiring a transactional DB.

          Nested transactions defeats this completely.

          Show
          Martín Langhoff added a comment - Petr, this is a problem specifically with the Files implementation. Do read my (pseudocode) example carefully, you will see the problem. In general, our move to transactions should get us in the right path towards (in some far future) using transactions properly, and requiring a transactional DB. Nested transactions defeats this completely.
          Hide
          Petr Škoda added a comment -

          I wrote it somewhere else already, I already used the transactions in extenal API in functions that take array as parameter (for example create users) and the execution somhow fails in the middle of the loop. What should we do then? Our function description expects only one type of return or exception, obvious solution was to rollback and return error instead of creating just half of the entries.

          Problem is that this new external API should be used in local customisations, custom Enrol and Auth plugins, etc - but if we have just one level we can not use transactions in these parts at all.

          I understand your example above, we should be imho moving away from file storage in dataroot, only files that are temporary or caches should be stored there - there should not be many rollback problems there I hope.

          Another problem I highlighted above are external events, they are imo much bigger problem then the files.

          Show
          Petr Škoda added a comment - I wrote it somewhere else already, I already used the transactions in extenal API in functions that take array as parameter (for example create users) and the execution somhow fails in the middle of the loop. What should we do then? Our function description expects only one type of return or exception, obvious solution was to rollback and return error instead of creating just half of the entries. Problem is that this new external API should be used in local customisations, custom Enrol and Auth plugins, etc - but if we have just one level we can not use transactions in these parts at all. I understand your example above, we should be imho moving away from file storage in dataroot, only files that are temporary or caches should be stored there - there should not be many rollback problems there I hope. Another problem I highlighted above are external events, they are imo much bigger problem then the files.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Another version:

          General principles:

          1. Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML)
          2. Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information.
          3. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback.
          4. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback.
          5. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level.
          6. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback()
          7. Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown.
          8. API will work in a natural and similar fashion to current recordsets one.

          The API:

          1. One array of objects of type moodle_nested_transaction will be stored / checked from $DB
          2. $DB will be the responsible to instantiate / accumulate / pair / compare moodle_nested_transactions
          3. Each moodle_nested_transaction will be able to set the global mark for rollback. Commit won't change anything.
          4. Normal usage of the moodle_nested_transaction will be:
            $transaction = $DB->start_transaction();
            // Perform some $DB stuff
            $transaction->commit();
            

          or

          $transaction = $DB->start_transaction();
          // Perform some $DB stuff
          $DB->commit($transaction);
          

          NOTE: To decide one of the alternatives.
          QUESTION: any error will end with one transaction_exception and one dml_exception? Not 100% sure here.

          1. If, for any reason, developer needs to catch dml_exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way:
            try {
                $transaction = $DB->start_transaction();
                // Perform some $DB stuff
                $transaction->commit();
            } catch (exception $e) {
                $transaction->rollback($e);
            }
            

          or

          try {
              $transaction = $DB->start_transaction($optional_name = null);
              // Perform some $DB stuff
              $DB->commit($transaction);
          } catch (exception $e) {
              $DB->rollback($transaction, $e);
          }
          

          NOTE: To decide one of the alternatives.

          The Flow:

          1. Any default exception handler will:
            1. Catch uncaught transaction_exception exceptions,
            2. Properly perform the DB rollback
            3. debug/error/log honouring related settings.
            4. inform with as many details as possible (token, place... whatever).
          2. Any "footer" (meaning some place before ending <html> output will:
            1. Detect "in-transaction" status.
            2. Throw transaction_exception (that will be caught, once again, by the default exception handler)
          3. moodle_request_shutdown will:
            1. Detect "in-transaction" status.
            2. debug/error/log honouring related settings (out from <html>!)
          4. $DB->dispose will:
            1. Detect "in-transaction" status.
            2. Properly perform the DB rollback
            3. log error (not possible to honour settings!)
            4. inform with as many details as possible (token, place... whatever).

          To explain/decide: the QUESTION and NOTEs above

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Another version: General principles: Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML) Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback() Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown. API will work in a natural and similar fashion to current recordsets one. The API: One array of objects of type moodle_nested_transaction will be stored / checked from $DB $DB will be the responsible to instantiate / accumulate / pair / compare moodle_nested_transactions Each moodle_nested_transaction will be able to set the global mark for rollback. Commit won't change anything. Normal usage of the moodle_nested_transaction will be: $transaction = $DB->start_transaction(); // Perform some $DB stuff $transaction->commit(); or $transaction = $DB->start_transaction(); // Perform some $DB stuff $DB->commit($transaction); NOTE: To decide one of the alternatives. QUESTION: any error will end with one transaction_exception and one dml_exception? Not 100% sure here. If, for any reason, developer needs to catch dml_exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way: try { $transaction = $DB->start_transaction(); // Perform some $DB stuff $transaction->commit(); } catch (exception $e) { $transaction->rollback($e); } or try { $transaction = $DB->start_transaction($optional_name = null ); // Perform some $DB stuff $DB->commit($transaction); } catch (exception $e) { $DB->rollback($transaction, $e); } NOTE: To decide one of the alternatives. The Flow: Any default exception handler will: Catch uncaught transaction_exception exceptions, Properly perform the DB rollback debug/error/log honouring related settings. inform with as many details as possible (token, place... whatever). Any "footer" (meaning some place before ending <html> output will: Detect "in-transaction" status. Throw transaction_exception (that will be caught, once again, by the default exception handler) moodle_request_shutdown will: Detect "in-transaction" status. debug/error/log honouring related settings (out from <html>!) $DB->dispose will: Detect "in-transaction" status. Properly perform the DB rollback log error (not possible to honour settings!) inform with as many details as possible (token, place... whatever). To explain/decide: the QUESTION and NOTEs above Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          About the syntax to decide. Simple: First 5 first opinions commented here will be taken into account. No more claims allowed later. ciao

          Show
          Eloy Lafuente (stronk7) added a comment - About the syntax to decide. Simple: First 5 first opinions commented here will be taken into account. No more claims allowed later. ciao
          Hide
          Petr Škoda added a comment -

          +1 for $transaction->commit()

          Show
          Petr Škoda added a comment - +1 for $transaction->commit()
          Hide
          Eloy Lafuente (stronk7) added a comment -

          voting for the 1st syntax, the one using the same approach that recordsets

          Show
          Eloy Lafuente (stronk7) added a comment - voting for the 1st syntax, the one using the same approach that recordsets
          Hide
          David Mudrak added a comment -

          Yep, the consistency with $rs = $DB->get_recordset_sql() sounds good. +1 for $transaction->commit()

          Show
          David Mudrak added a comment - Yep, the consistency with $rs = $DB->get_recordset_sql() sounds good. +1 for $transaction->commit()
          Hide
          Eloy Lafuente (stronk7) added a comment -

          3 votes = will be using the like recordsets syntax. Reworking the specs once more

          Show
          Eloy Lafuente (stronk7) added a comment - 3 votes = will be using the like recordsets syntax. Reworking the specs once more
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Another version:

          General principles:

          1. Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML)
          2. Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information.
          3. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback.
          4. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback.
          5. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level.
          6. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback()
          7. Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown.
          8. API will work in a natural and similar fashion to current recordsets one.
          9. We'll start recommending to use InnoDB for 2.0 in environmental tests. Posibly, at some point in the future that will become a must. But it is not for now (be warned!).

          The API:

          1. One array of objects of type moodle_nested_transaction will be stored / checked from $DB
          2. $DB will be the responsible to instantiate / accumulate / pair / compare moodle_nested_transactions
          3. Each moodle_nested_transaction will be able to set the global mark for rollback. Commit won't change anything.
          4. Normal usage of the moodle_nested_transaction will be:
            $transaction = $DB->start_transaction();
            // Perform some $DB stuff
            $transaction->commit();
            
          1. If, for any reason, developer needs to catch dml_exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way:
            try {
                $transaction = $DB->start_transaction();
                // Perform some $DB stuff
                $transaction->commit();
            } catch (exception $e) {
                $transaction->rollback($e);
            }
            

          The Flow:

          1. Any default exception handler will:
            1. Catch uncaught transaction_exception exceptions,
            2. Properly perform the DB rollback
            3. debug/error/log honouring related settings.
            4. inform with as many details as possible (token, place... whatever).
          2. Any "footer" (meaning some place before ending <html> output) will:
            1. Detect "in-transaction" status.
            2. Throw transaction_exception (that will be caught, once again, by the default exception handler)
          3. moodle_request_shutdown will:
            1. Detect "in-transaction" status.
            2. debug/error/log honouring related settings (out from <html>!)
          4. $DB->dispose will:
            1. Detect "in-transaction" status.
            2. Properly perform the DB rollback
            3. log error (not possible to honour settings!)
            4. inform with as many details as possible (token, place... whatever).

          Last iteration, I hope. Please review once more.

          Show
          Eloy Lafuente (stronk7) added a comment - Another version: General principles: Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML) Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback() Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown. API will work in a natural and similar fashion to current recordsets one. We'll start recommending to use InnoDB for 2.0 in environmental tests. Posibly, at some point in the future that will become a must . But it is not for now (be warned!). The API: One array of objects of type moodle_nested_transaction will be stored / checked from $DB $DB will be the responsible to instantiate / accumulate / pair / compare moodle_nested_transactions Each moodle_nested_transaction will be able to set the global mark for rollback. Commit won't change anything. Normal usage of the moodle_nested_transaction will be: $transaction = $DB->start_transaction(); // Perform some $DB stuff $transaction->commit(); If, for any reason, developer needs to catch dml_exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way: try { $transaction = $DB->start_transaction(); // Perform some $DB stuff $transaction->commit(); } catch (exception $e) { $transaction->rollback($e); } The Flow: Any default exception handler will: Catch uncaught transaction_exception exceptions, Properly perform the DB rollback debug/error/log honouring related settings. inform with as many details as possible (token, place... whatever). Any "footer" (meaning some place before ending <html> output) will: Detect "in-transaction" status. Throw transaction_exception (that will be caught, once again, by the default exception handler) moodle_request_shutdown will: Detect "in-transaction" status. debug/error/log honouring related settings (out from <html>!) $DB->dispose will: Detect "in-transaction" status. Properly perform the DB rollback log error (not possible to honour settings!) inform with as many details as possible (token, place... whatever). Last iteration, I hope. Please review once more.
          Hide
          Martín Langhoff added a comment -

          Hi Petr,

          I am not talking about dataroot. I am talking about how to code the internal implementation of our file storage. If we use nested transactions, we never know of the $db->begin(); DELETE .FROM ... ; $db->commit(); happens or not.

          Unless we poke the $db object to ask "are we in a tx already?" and error out if we are. Any code that has any kind of side-effect has to refuse to work if nested tx are in use.

          Show
          Martín Langhoff added a comment - Hi Petr, I am not talking about dataroot. I am talking about how to code the internal implementation of our file storage. If we use nested transactions, we never know of the $db->begin(); DELETE .FROM ... ; $db->commit(); happens or not. Unless we poke the $db object to ask "are we in a tx already?" and error out if we are. Any code that has any kind of side-effect has to refuse to work if nested tx are in use.
          Hide
          Petr Škoda added a comment - - edited

          our internal implementation of file storage survives the DB transactions, we are not deleting immediately the orphaned file contents, and if you add new file content the worst case is it will be slowly accumulating there, most probably you will retry the attempt later and the file would be used then

          Show
          Petr Škoda added a comment - - edited our internal implementation of file storage survives the DB transactions, we are not deleting immediately the orphaned file contents, and if you add new file content the worst case is it will be slowly accumulating there, most probably you will retry the attempt later and the file would be used then
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, FileAPI (100% luckily won't be really exposed to problems like that). But others could. So perhaps we should prevent transactions at all when dealing with external systems or alternative storage places. Say... mailouts for example.

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, FileAPI (100% luckily won't be really exposed to problems like that). But others could. So perhaps we should prevent transactions at all when dealing with external systems or alternative storage places. Say... mailouts for example.
          Hide
          Petr Škoda added a comment -

          well, the new notifications framework should definitely deal with this somehow (this includes mailing)

          Show
          Petr Škoda added a comment - well, the new notifications framework should definitely deal with this somehow (this includes mailing)
          Hide
          Martin Dougiamas added a comment -

          +1 for Eloy's last spec, because I like his smile.

          Show
          Martin Dougiamas added a comment - +1 for Eloy's last spec, because I like his smile.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          So the last version, after some chats is here:

          Another version:

          General principles:

          1. Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML)
          2. Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information.
          3. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback.
          4. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback.
          5. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level.
          6. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback()
          7. Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown.
          8. API will work in a natural and similar fashion to current recordsets one.
          9. We'll start recommending to use InnoDB for 2.0 in environmental tests. Posibly, at some point in the future that will become a must. But it is not for now (be warned!).

          The API:

          1. All the handling must go, exclusively, to moodle_database object, leaving real drivers only implementing (protected) the old begin/commit/rollback_sql() functions.
          2. One array of objects of type moodle_nested_transaction will be stored / checked from $DB
          3. $DB will be the responsible to instantiate / accumulate / pair / compare moodle_nested_transactions
          4. Each moodle_nested_transaction will be able to set the global mark for rollback. Commit won't change anything.
          5. Normal usage of the moodle_nested_transaction will be:
            $transaction = $DB->start_transaction();
            // Perform some $DB stuff
            $transaction->commit();
            
          1. If, for any reason, developer needs to catch exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way:
            try {
                $transaction = $DB->start_transaction();
                // Perform some $DB stuff
                $transaction->commit();
            } catch (exception $e) {
                $transaction->rollback($e);
            }
            

          The Flow:

          1. Any default exception handler will:
            1. Catch uncaught transaction_exception exceptions,
            2. Properly perform the DB rollback
            3. debug/error/log honouring related settings.
            4. inform with as many details as possible (token, place... whatever).
          2. Any "footer" (meaning some place before ending <html> output) will:
            1. Detect "in-transaction" status.
            2. Throw transaction_exception (that will be caught, once again, by the default exception handler)
          3. moodle_request_shutdown will:
            1. Detect "in-transaction" status.
            2. debug/error/log honouring related settings (out from <html>!)
          4. $DB->dispose will:
            1. Detect "in-transaction" status.
            2. Properly perform the DB rollback
            3. log error (not possible to honour settings!)
            4. inform with as many details as possible (token, place... whatever).

          Related tasks:

          1. Move this to MoodleDocs and publicy in Dev Forum.
          2. Implement fulfilling current specs / review / test iterations
          3. Re-enforce in Docs, phpdocs, everywhere, the "Code will never rely on rollback happening"
          4. Be extremely careful about any use of nested transactions until we get used to them, reviewing them in core often.
          5. Point somewhere about the move to InnoDB, pros/cons, migration process... to have it documented.

          Changes from previous version:

          • Added "The API" point 1.
          • Added "Related tasks" section.
          Show
          Eloy Lafuente (stronk7) added a comment - So the last version, after some chats is here: Another version: General principles: Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML) Code will never rely on rollback happening. it's only a measure to reduce (not to eliminate) DB garbled information. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback() Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown. API will work in a natural and similar fashion to current recordsets one. We'll start recommending to use InnoDB for 2.0 in environmental tests. Posibly, at some point in the future that will become a must . But it is not for now (be warned!). The API: All the handling must go, exclusively, to moodle_database object, leaving real drivers only implementing (protected) the old begin/commit/rollback_sql() functions. One array of objects of type moodle_nested_transaction will be stored / checked from $DB $DB will be the responsible to instantiate / accumulate / pair / compare moodle_nested_transactions Each moodle_nested_transaction will be able to set the global mark for rollback. Commit won't change anything. Normal usage of the moodle_nested_transaction will be: $transaction = $DB->start_transaction(); // Perform some $DB stuff $transaction->commit(); If, for any reason, developer needs to catch exceptions when using moodle_nested_transaction, it will be mandatory to use it in this way: try { $transaction = $DB->start_transaction(); // Perform some $DB stuff $transaction->commit(); } catch (exception $e) { $transaction->rollback($e); } The Flow: Any default exception handler will: Catch uncaught transaction_exception exceptions, Properly perform the DB rollback debug/error/log honouring related settings. inform with as many details as possible (token, place... whatever). Any "footer" (meaning some place before ending <html> output) will: Detect "in-transaction" status. Throw transaction_exception (that will be caught, once again, by the default exception handler) moodle_request_shutdown will: Detect "in-transaction" status. debug/error/log honouring related settings (out from <html>!) $DB->dispose will: Detect "in-transaction" status. Properly perform the DB rollback log error (not possible to honour settings!) inform with as many details as possible (token, place... whatever). Related tasks: Move this to MoodleDocs and publicy in Dev Forum. Implement fulfilling current specs / review / test iterations Re-enforce in Docs, phpdocs, everywhere, the "Code will never rely on rollback happening" Be extremely careful about any use of nested transactions until we get used to them, reviewing them in core often. Point somewhere about the move to InnoDB, pros/cons, migration process... to have it documented. Changes from previous version: Added "The API" point 1. Added "Related tasks" section.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My +1 too.

          Show
          Eloy Lafuente (stronk7) added a comment - My +1 too.
          Hide
          Martín Langhoff added a comment -

          I am ok with the proposed transaction API, and exceptions approach.

          But I am not convinced at all about the nesting:

          • In enabling "nesting", we completely mess up transactions. $db->commit() becomes $db->maybe_commit(). This means we cannot sync with any external state and generally is NOT what transactions are about, and blocks the way forward to real transactions-based Moodle.
          • Nobody has put forth an example where nested transactions are beneficial.

          Please please please, either skip nested tx or at the very least make moodle gargle very loud and ugly complaints when nested transactions are used.

          Show
          Martín Langhoff added a comment - I am ok with the proposed transaction API, and exceptions approach. But I am not convinced at all about the nesting: In enabling "nesting", we completely mess up transactions. $db->commit() becomes $db->maybe_commit(). This means we cannot sync with any external state and generally is NOT what transactions are about, and blocks the way forward to real transactions-based Moodle. Nobody has put forth an example where nested transactions are beneficial. Please please please, either skip nested tx or at the very least make moodle gargle very loud and ugly complaints when nested transactions are used.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Martin,

          as commented repeatedly I agree with you I don't buy the "nested transactions" concept. 100%

          And agree the commit() becomes maybe_commit(), in fact it becomes do_nothing(), because final decision of real commit becomes delayed until top level is closed. That was my initial pov about to remove the "commit" and "rollback" words from the API, but finally was agreed to have them there. It's just a matter of knowing what they mean.

          About uses, well, I can see the point about, for example, making the creation of one course, as one transaction (it involves creating various records in various tables) and any failure in the process being nicely reverted sounds ok (normal 1-level transaction).

          But problems arise when I'm trying to use that create_course() function in web services, or in restore or bulk creation of courses... where I also want to handle transactions. For example, I want to make the whole restore process to run in a transaction, it creates zillions of records and any failure is really an (almost invisible luckily) mess. But, with real 1-level transactions I'll be forbidden to use create_course(), while with the "delegated transactions" (uhm, perhaps whe should name them that way!) I'll be able to do so and restore will have the control.

          In any case, note that we never will rely in transaction status (no matter if they are 1-level real ones or delegated/nested ones). Mainly because not all our DBs support them. It's ONLY one measure to reduce the mess in DB and not to eliminate it! Only to reduce! It's only one anti-mess-feature for DBs supporting them. And, as said, this principle is the same, no matter if we are using 1-level or n-level ones.

          I think we need to be 300% careful before start using them and just then we'll discover real problems (if they exist). But, in the other side, all reports I've found from ADOdb, Mahara, OU... comment they are working ok (and I'd add: "if under control"). That's the key, preventing any use related to "external subsystems" will help, for sure.

          Finally, 100% agree about your idea of "moodle gargle very loud and ugly complaints when nested transactions are used". Not because of complaints, but to let the developer know he's executing code that have transactions within it. So, perhaps I'd add one new debug parameter, enabled by default, call it "show nested transactions stacks", so, in DEBUG_DEVELOPER mode, it will be showing the whole transactions stack when the inner one is commit/rollback. Definitively that will help.

          So, having the feature will lead us to be able to use it, surely as 1-level only in most places initially (islands), but with the possibility of allowing caller code to take the control in the future (delegated).

          Uhm... the most I think the most I like the "delegated" concept.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Martin, as commented repeatedly I agree with you I don't buy the "nested transactions" concept. 100% And agree the commit() becomes maybe_commit(), in fact it becomes do_nothing(), because final decision of real commit becomes delayed until top level is closed. That was my initial pov about to remove the "commit" and "rollback" words from the API, but finally was agreed to have them there. It's just a matter of knowing what they mean. About uses, well, I can see the point about, for example, making the creation of one course, as one transaction (it involves creating various records in various tables) and any failure in the process being nicely reverted sounds ok (normal 1-level transaction). But problems arise when I'm trying to use that create_course() function in web services, or in restore or bulk creation of courses... where I also want to handle transactions. For example, I want to make the whole restore process to run in a transaction, it creates zillions of records and any failure is really an (almost invisible luckily) mess. But, with real 1-level transactions I'll be forbidden to use create_course(), while with the "delegated transactions" (uhm, perhaps whe should name them that way!) I'll be able to do so and restore will have the control. In any case, note that we never will rely in transaction status (no matter if they are 1-level real ones or delegated/nested ones). Mainly because not all our DBs support them. It's ONLY one measure to reduce the mess in DB and not to eliminate it! Only to reduce! It's only one anti-mess-feature for DBs supporting them. And, as said, this principle is the same, no matter if we are using 1-level or n-level ones. I think we need to be 300% careful before start using them and just then we'll discover real problems (if they exist). But, in the other side, all reports I've found from ADOdb, Mahara, OU... comment they are working ok (and I'd add: "if under control"). That's the key, preventing any use related to "external subsystems" will help, for sure. Finally, 100% agree about your idea of "moodle gargle very loud and ugly complaints when nested transactions are used". Not because of complaints, but to let the developer know he's executing code that have transactions within it. So, perhaps I'd add one new debug parameter, enabled by default, call it "show nested transactions stacks", so, in DEBUG_DEVELOPER mode, it will be showing the whole transactions stack when the inner one is commit/rollback. Definitively that will help. So, having the feature will lead us to be able to use it, surely as 1-level only in most places initially (islands), but with the possibility of allowing caller code to take the control in the future (delegated). Uhm... the most I think the most I like the "delegated" concept. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          New specs introducing the "delegate" concept everywhere:

          General principles:

          1. Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML)
          2. No matter of they nested nature, they are really "delegated transactions" (so outer levels get the control over inner ones), so we are going to apply that naming to enforce the real concept.
          3. Code will never rely on rollback happening. it's only a maeasure to reduce (not to eliminate) DB garbled information.
          4. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback.
          5. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback.
          6. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level.
          7. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback()
          8. Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown.
          9. API will work in a natural and similar fashion to current recordsets one.
          10. We'll start recommending to use InnoDB for 2.0 in environmental tests. Posibly, at some point in the future that will become a must. But it is not for now (be warned!).

          The API:

          1. All the handling must go, exclusively, to moodle_database object, leaving real drivers only implementing (protected) the old begin/commit/rollback_sql() functions.
          2. One array of objects of type moodle_delegated_transaction will be stored / checked from $DB
          3. $DB will be the responsible to instantiate / accumulate / pair / compare moodle_delegated_transactions
          4. Each moodle_delegated_transaction will be able to set the global mark for rollback. Commit won't change anything.
          5. Inner-most commit/rollback will printout one complete stack of moodle_delegated_transactions information if we are under DEBUG_DEVELOPER and new setting (delegatedtransactionsdebug) is enabled.
          6. Normal usage of the moodle_delegated_transaction will be:
            $delegated_transaction = $DB->start_delegated_transaction();
            // Perform some $DB stuff
            $delegated_transaction->delegated_commit();
            
          1. If, for any reason, developer needs to catch exceptions when using moodle_delegated_transactions, it will be mandatory to use it in this way:
            try {
                $delegated_transaction = $DB->start_delegated_transaction();
                // Perform some $DB stuff
                $delegated_transaction->delegated_commit();
            } catch (exception $e) {
                $delegated_transaction->delegated_rollback($e);
            }
            

          (note I've abused the "delegated" concept in the examples above, just to enforce it)

          The Flow:

          1. Any default exception handler will:
            1. Catch uncaught transaction_exception exceptions,
            2. Properly perform the DB rollback
            3. debug/error/log honouring related settings.
            4. inform with as many details as possible (token, place... whatever).
          2. Any "footer" (meaning some place before ending <html> output) will:
            1. Detect "in-transaction" status.
            2. Throw transaction_exception (that will be caught, once again, by the default exception handler)
          3. moodle_request_shutdown will:
            1. Detect "in-transaction" status.
            2. debug/error/log honouring related settings (out from <html>!)
          4. $DB->dispose will:
            1. Detect "in-transaction" status.
            2. Properly perform the DB rollback
            3. log error (not possible to honour settings!)
            4. inform with as many details as possible (token, place... whatever).

          Related tasks:

          1. Move this to MoodleDocs and publicy in Dev Forum.
          2. Implement fulfilling current specs / review / test iterations
          3. Re-enforce in Docs, phpdocs, everywhere, the "Code will never rely on rollback happening" and the "delegated" nature of the whole thing.
          4. Be extremely careful about any use of delegated transactions until we get used to them, reviewing them in core often.
          5. Point somewhere about the move to InnoDB, pros/cons, migration process... to have it documented.

          Changes from previous version:

          • Chaging from "nested transactions" to "delegated transactions" everywhere.
          • Introducing new setting to print-out transactions stack on inner-most commit/rollback.
          Show
          Eloy Lafuente (stronk7) added a comment - - edited New specs introducing the "delegate" concept everywhere: General principles: Everybody thinks it's a good idea to have those logical nested transactions in Moodle (but me and ML) No matter of they nested nature, they are really "delegated transactions" (so outer levels get the control over inner ones), so we are going to apply that naming to enforce the real concept. Code will never rely on rollback happening. it's only a maeasure to reduce (not to eliminate) DB garbled information. Any problem related with the use of transactions (unfinished ones, unbalanced, finished twice...) will end always with one transaction_exception that will always end performing one DB rollback. If one transaction (at any level) has been marked for rollback() there won't be any method to change it. Finally Moodle will perform the DB rollback. If one transaction (at any level) has been marked for commit() it will be possible to change that status to rollback() in any outer level. It will be optional to catch exceptions when using transactions, but if they are caught, then it's mandatory to mark the transaction for rollback() Any explicit rollback() call will pass the exception originating it, i.e. rollback($exception) to be re-thrown. API will work in a natural and similar fashion to current recordsets one. We'll start recommending to use InnoDB for 2.0 in environmental tests. Posibly, at some point in the future that will become a must . But it is not for now (be warned!). The API: All the handling must go, exclusively, to moodle_database object, leaving real drivers only implementing (protected) the old begin/commit/rollback_sql() functions. One array of objects of type moodle_delegated_transaction will be stored / checked from $DB $DB will be the responsible to instantiate / accumulate / pair / compare moodle_delegated_transactions Each moodle_delegated_transaction will be able to set the global mark for rollback. Commit won't change anything. Inner-most commit/rollback will printout one complete stack of moodle_delegated_transactions information if we are under DEBUG_DEVELOPER and new setting (delegatedtransactionsdebug) is enabled. Normal usage of the moodle_delegated_transaction will be: $delegated_transaction = $DB->start_delegated_transaction(); // Perform some $DB stuff $delegated_transaction->delegated_commit(); If, for any reason, developer needs to catch exceptions when using moodle_delegated_transactions, it will be mandatory to use it in this way: try { $delegated_transaction = $DB->start_delegated_transaction(); // Perform some $DB stuff $delegated_transaction->delegated_commit(); } catch (exception $e) { $delegated_transaction->delegated_rollback($e); } (note I've abused the "delegated" concept in the examples above, just to enforce it) The Flow: Any default exception handler will: Catch uncaught transaction_exception exceptions, Properly perform the DB rollback debug/error/log honouring related settings. inform with as many details as possible (token, place... whatever). Any "footer" (meaning some place before ending <html> output) will: Detect "in-transaction" status. Throw transaction_exception (that will be caught, once again, by the default exception handler) moodle_request_shutdown will: Detect "in-transaction" status. debug/error/log honouring related settings (out from <html>!) $DB->dispose will: Detect "in-transaction" status. Properly perform the DB rollback log error (not possible to honour settings!) inform with as many details as possible (token, place... whatever). Related tasks: Move this to MoodleDocs and publicy in Dev Forum. Implement fulfilling current specs / review / test iterations Re-enforce in Docs, phpdocs, everywhere, the "Code will never rely on rollback happening" and the "delegated" nature of the whole thing. Be extremely careful about any use of delegated transactions until we get used to them, reviewing them in core often. Point somewhere about the move to InnoDB, pros/cons, migration process... to have it documented. Changes from previous version: Chaging from "nested transactions" to "delegated transactions" everywhere. Introducing new setting to print-out transactions stack on inner-most commit/rollback.
          Hide
          Sam Marshall added a comment -

          by the way, yay proposed API.

          martin L - examples for this sort of thing are really, really easy. it's a coding pattern (which you would no doubt call an antipattern)...

          function does_something() {
            // makes 2 db changes, eg insert record and update another to point to it
          }
          

          so clearly function does_something should have a transaction because it would leave the database in a corrupt state if only one of those two db changes happens. now let's say it can be called on its own by end-user code, or else:

          function does_something_complicated() {
            does_something();
            does_something_else();
          } 
          

          Again, this operation contains two tasks both of which make db changes. You don't want to leave the database in a half-finished state so it is important that it's in a transaction... etc...

          Really the nested transactions are not so much nested transactions (because that's impossible). The API is a way of saying 'I require that there is a single transaction covering these statements' (which might be a transaction just for them, if you called does_something() directly, or a larger-scale transaction, if you called does_something_complicated().)

          I agree there are problems when transactions cover actions with external side-effects that cannot be rolled back but I think these should be manageable.

          Perhaps there are some API extensions that might assist this (I am speculating not recommending necessarily):

          $DB->require_no_transaction();
          

          Calls print_error if there is a transaction.

          ALTERNATIVE API:

          $transaction = $DB->start_top_level_transaction();
          

          (or start_transaction(true) or whatever)

          Same as start_transaction but throws error if there is a transaction already underway

          $DB->add_rollback_handler($callback, $param);
          $DB->add_commit_handler($callback, $param);
          

          Gives error if there is no transaction.
          If there is a transaction, adds callback to a list.
          If transaction is rolled back or committed, carries out callback.

          This could be used for things like unlink() where you don't mind if it hangs around and does it only after all db activity is successful. Obviously it would not address the problem in something like cron sending out emails or anything that needs to do a large quantity of work, for that case require_no_transaction would be more appropriate. And it cannot of course solve the problem where you really want both things to be guaranteed on each other, i.e. if the unlink fails then you want the database not to have gone ahead.

          Also: I don't think we should add debug information that's on by default for nested transactions. If doing something like that then there should be a way of preventing it in code otherwise it becomes a meaningless warning - even if original developer sees warning and checks it is okay, other developers would see the warning and not know whether it's a problem or not. For example it might be possible to specifically tell a transaction that it is OK if it has parent transactions (thus turning off the warning).

          Show
          Sam Marshall added a comment - by the way, yay proposed API. martin L - examples for this sort of thing are really, really easy. it's a coding pattern (which you would no doubt call an antipattern)... function does_something() { // makes 2 db changes, eg insert record and update another to point to it } so clearly function does_something should have a transaction because it would leave the database in a corrupt state if only one of those two db changes happens. now let's say it can be called on its own by end-user code, or else: function does_something_complicated() { does_something(); does_something_else(); } Again, this operation contains two tasks both of which make db changes. You don't want to leave the database in a half-finished state so it is important that it's in a transaction... etc... Really the nested transactions are not so much nested transactions (because that's impossible). The API is a way of saying 'I require that there is a single transaction covering these statements' (which might be a transaction just for them, if you called does_something() directly, or a larger-scale transaction, if you called does_something_complicated().) I agree there are problems when transactions cover actions with external side-effects that cannot be rolled back but I think these should be manageable. Perhaps there are some API extensions that might assist this (I am speculating not recommending necessarily): $DB->require_no_transaction(); Calls print_error if there is a transaction. ALTERNATIVE API: $transaction = $DB->start_top_level_transaction(); (or start_transaction(true) or whatever) Same as start_transaction but throws error if there is a transaction already underway $DB->add_rollback_handler($callback, $param); $DB->add_commit_handler($callback, $param); Gives error if there is no transaction. If there is a transaction, adds callback to a list. If transaction is rolled back or committed, carries out callback. This could be used for things like unlink() where you don't mind if it hangs around and does it only after all db activity is successful. Obviously it would not address the problem in something like cron sending out emails or anything that needs to do a large quantity of work, for that case require_no_transaction would be more appropriate. And it cannot of course solve the problem where you really want both things to be guaranteed on each other, i.e. if the unlink fails then you want the database not to have gone ahead. Also: I don't think we should add debug information that's on by default for nested transactions. If doing something like that then there should be a way of preventing it in code otherwise it becomes a meaningless warning - even if original developer sees warning and checks it is okay, other developers would see the warning and not know whether it's a problem or not. For example it might be possible to specifically tell a transaction that it is OK if it has parent transactions (thus turning off the warning).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          yeah, Sam, I was thinking about to be able to differentiate outer "transactions" from the rest some minutes ago (using it to mark places than cannot be delegated and are mandatory top level). But finally didn't wrote about that to keep the "getter" uniform.

          But I like a lot the:

          $DB->delegated_transaction_allowed(true/false); (or similar)

          to be able to mark places where transactions cannot be used. I like that a lot!!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - yeah, Sam, I was thinking about to be able to differentiate outer "transactions" from the rest some minutes ago (using it to mark places than cannot be delegated and are mandatory top level). But finally didn't wrote about that to keep the "getter" uniform. But I like a lot the: $DB->delegated_transaction_allowed(true/false); (or similar) to be able to mark places where transactions cannot be used. I like that a lot!! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki, so we are talking about to have (only using the long API to show all methods):

          try {
              $transaction = $DB->start_delegated_transaction();
              // Perform some $DB stuff
              $transaction->delegated_commit();
          } catch (exception $e) {
              $transaction->delegated_rollback($e);
          }
          

          or, alternatively:

          try {
              $transaction = $DB-> start_delegated_transaction();
              // Perform some $DB stuff
              $transaction->allow_commit(); // To enforce the "this might be a real commit or no. I'm just authorising it"
          } catch (exception $e) {
              $transaction->rollback($e); // To enforce the "This is going to be reverted (rollback). I've decided"
          }
          

          And also, have the possibility to specify:

          $DB->transaction_required();
          $DB->transaction_forbidden();
          $DB->transaction_allowed();

          (in some better way, of course)

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki, so we are talking about to have (only using the long API to show all methods): try { $transaction = $DB->start_delegated_transaction(); // Perform some $DB stuff $transaction->delegated_commit(); } catch (exception $e) { $transaction->delegated_rollback($e); } or, alternatively: try { $transaction = $DB-> start_delegated_transaction(); // Perform some $DB stuff $transaction->allow_commit(); // To enforce the " this might be a real commit or no. I'm just authorising it" } catch (exception $e) { $transaction->rollback($e); // To enforce the "This is going to be reverted (rollback). I've decided" } And also, have the possibility to specify: $DB->transaction_required(); $DB->transaction_forbidden(); $DB->transaction_allowed(); (in some better way, of course)
          Hide
          Sam Marshall added a comment -

          I like the 'alternatively' version [Not a fan of 'delegated']

          +1 for transaction_required / forbidden

          what does transaction_allowed do?

          Show
          Sam Marshall added a comment - I like the 'alternatively' version [Not a fan of 'delegated'] +1 for transaction_required / forbidden what does transaction_allowed do?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          disables (reset) the required/forbidden status, i.e. developer is free to use or no transactions.

          And I' would keep the "delegated" concept in the getter.

          Edited: Naaah! required hasn't sense, just use them. Only forbidden/allowed has. grrr

          Show
          Eloy Lafuente (stronk7) added a comment - - edited disables (reset) the required/forbidden status, i.e. developer is free to use or no transactions. And I' would keep the "delegated" concept in the getter. Edited: Naaah! required hasn't sense, just use them. Only forbidden/allowed has. grrr
          Hide
          Sam Marshall added a comment - - edited

          Another option for the start method (yes I am talking about naming again) would be

          $transactionref = $DB->require_transaction();
          

          i.e. you are not 'start' - ing a new transaction, you are just requiring that one has been started (either now or before). and what you get back isn't a 'transaction' object but some kind of reference referring to a transaction which is managed by $DB.

          I thought required/forbidden would be 'instant' methods, to be used like this (w/ my name) if you want to ensure that your transaction is a 'real' one:

          $DB->transaction_forbidden();
          $transactionref = $DB->require_transaction();
          ...
          $transactionref->allow_commit();
          // no need for allow method
          

          however in this case it might be better to have a different method

          $transactionref = $DB->start_transaction(); // throws error if there is already one
          ...
          $transactionref->commit(); // instead of 'allow_commit', allowed only if you used start_transaction
          

          If it wasn't clear, the idea was, it's actually the same code, but different function names (exactly same behaviour but with check for no previous transaction) so that you have API for either a 'nested' style transaction or a 'Martin L' style dictatorial transaction

          anyhow I think this is edge stuff, overall eloy's last looks good to me.

          Show
          Sam Marshall added a comment - - edited Another option for the start method (yes I am talking about naming again) would be $transactionref = $DB->require_transaction(); i.e. you are not 'start' - ing a new transaction, you are just requiring that one has been started (either now or before). and what you get back isn't a 'transaction' object but some kind of reference referring to a transaction which is managed by $DB. I thought required/forbidden would be 'instant' methods, to be used like this (w/ my name) if you want to ensure that your transaction is a 'real' one: $DB->transaction_forbidden(); $transactionref = $DB->require_transaction(); ... $transactionref->allow_commit(); // no need for allow method however in this case it might be better to have a different method $transactionref = $DB->start_transaction(); // throws error if there is already one ... $transactionref->commit(); // instead of 'allow_commit', allowed only if you used start_transaction If it wasn't clear, the idea was, it's actually the same code, but different function names (exactly same behaviour but with check for no previous transaction) so that you have API for either a 'nested' style transaction or a 'Martin L' style dictatorial transaction anyhow I think this is edge stuff, overall eloy's last looks good to me.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Sam,

          I think it's better to use always the same: $DB-> start_delegated_transaction() to get one transaction (real or delegated), it doesn't matter.

          And then, have the $DB->transaction_forbidden(); instant check apart, because they aren't always related (the check and the creation of one real - level1, transaction). For example, parts of code exposed to problems like the commented by ML, that we know can be causing problems under transactions, will be enclosed simply by $DB->transaction_forbidden() and $DB->transaction_allowed() and the code, obviously won't be trying to create any transaction, as far as it's know to cause problems, but we'll have the code "isolated" from being used in transactions.

          Summarising, check + getter together aren't always the expected use. Let's keep them separated.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Sam, I think it's better to use always the same: $DB-> start_delegated_transaction() to get one transaction (real or delegated), it doesn't matter. And then, have the $DB->transaction_forbidden(); instant check apart, because they aren't always related (the check and the creation of one real - level1, transaction). For example, parts of code exposed to problems like the commented by ML, that we know can be causing problems under transactions, will be enclosed simply by $DB->transaction_forbidden() and $DB->transaction_allowed() and the code, obviously won't be trying to create any transaction, as far as it's know to cause problems, but we'll have the code "isolated" from being used in transactions. Summarising, check + getter together aren't always the expected use. Let's keep them separated. Ciao
          Hide
          Martín Langhoff added a comment -

          I like the idea of separating "hard", must-be-top-level transactions from "soft", "delegated" transactions.

          I don't care too much about the name itself, I like anything that reserves "begin()", "rollback()" and "transaction()" for the hard, real database transactions. Principle of least suprise for newcomers, and hwen reading the code things make sense.

          Maybe "soft" is a good alternative for a name? $DB->start_soft_transaction()

          Show
          Martín Langhoff added a comment - I like the idea of separating "hard", must-be-top-level transactions from "soft", "delegated" transactions. I don't care too much about the name itself, I like anything that reserves "begin()", "rollback()" and "transaction()" for the hard, real database transactions. Principle of least suprise for newcomers, and hwen reading the code things make sense. Maybe "soft" is a good alternative for a name? $DB->start_soft_transaction()
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Moved current specs to: http://docs.moodle.org/en/Development:DB_layer_2.0_delegated_transactions

          Let's continue discussing in forum, as Tim has re-started it at http://moodle.org/mod/forum/discuss.php?d=135847#p596749

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Moved current specs to: http://docs.moodle.org/en/Development:DB_layer_2.0_delegated_transactions Let's continue discussing in forum, as Tim has re-started it at http://moodle.org/mod/forum/discuss.php?d=135847#p596749 Ciao
          Hide
          Petr Škoda added a comment - - edited

          sending new unfinished patch - mostly missing comments, should work fine, unittests included

          I hope it fulfils all requirements

          ciao

          Show
          Petr Škoda added a comment - - edited sending new unfinished patch - mostly missing comments, should work fine, unittests included I hope it fulfils all requirements ciao
          Hide
          Petr Škoda added a comment -

          Eloy, could you please review my latest patch? Please concentrate on the implementation, not the actual names of methods, we can change them later

          Show
          Petr Škoda added a comment - Eloy, could you please review my latest patch? Please concentrate on the implementation, not the actual names of methods, we can change them later
          Hide
          Petr Škoda added a comment -

          updated after feedback from Eloy

          Show
          Petr Škoda added a comment - updated after feedback from Eloy
          Hide
          Petr Škoda added a comment -

          some more TODOs for tim in patch

          Show
          Petr Škoda added a comment - some more TODOs for tim in patch
          Hide
          Tim Hunt added a comment -

          I guess http://tracker.moodle.org/secure/attachment/18803/delegated_transactions_20091104_3.patch is the patch I should be looking at. I'll have to look later.

          Show
          Tim Hunt added a comment - I guess http://tracker.moodle.org/secure/attachment/18803/delegated_transactions_20091104_3.patch is the patch I should be looking at. I'll have to look later.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My +1

          Petr just three comments:

          1) In the mysqli driver, rollback_transaction() method, there is one old call to parent::rollback_sql(). Must be out.
          2) Tests passes ok in all DBs (but mysqli - MyISAM, of course). About this... do we have any method (or would be useful) about transactions being supported or no? Much like the sql_regex_supported() or so, just to know. I think it can be private and, in our moodle_database_for_testing() be able to check it and output better messages in tests.
          3) Also, in tests, I think it would be great to be able to open a new connection to DB, call it DB2, in order to:

          • Test that the "force new connection. Is working in all drivers.
          • Have connection code covered by tests.
          • Check that transactions are isolated as they are expected to be (read committed).

          And that's all, as said, my +1

          Then it will be a matter of:

          • Document new public API and migration.
          • Perhaps document some uses in the Examples.
          • Change the db_checker to detect old function uses.
          • Implement the "footer catcher" as specified in Docs, I think you confirmed me that it's pending because of some ongoing work in output-world or so.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - My +1 Petr just three comments: 1) In the mysqli driver, rollback_transaction() method, there is one old call to parent::rollback_sql(). Must be out. 2) Tests passes ok in all DBs (but mysqli - MyISAM, of course). About this... do we have any method (or would be useful) about transactions being supported or no? Much like the sql_regex_supported() or so, just to know. I think it can be private and, in our moodle_database_for_testing() be able to check it and output better messages in tests. 3) Also, in tests, I think it would be great to be able to open a new connection to DB, call it DB2, in order to: Test that the "force new connection. Is working in all drivers. Have connection code covered by tests. Check that transactions are isolated as they are expected to be (read committed). And that's all, as said, my +1 Then it will be a matter of: Document new public API and migration. Perhaps document some uses in the Examples. Change the db_checker to detect old function uses. Implement the "footer catcher" as specified in Docs, I think you confirmed me that it's pending because of some ongoing work in output-world or so. Ciao
          Hide
          Petr Škoda added a comment -

          1/ patch committed
          2/ transactions are now forbidden when sending messages
          3/ instant events are queued when in transaction - this is slow, but should work 100% for external systems dependent on event notifications

          going to work more on unit tests and docs

          Show
          Petr Škoda added a comment - 1/ patch committed 2/ transactions are now forbidden when sending messages 3/ instant events are queued when in transaction - this is slow, but should work 100% for external systems dependent on event notifications going to work more on unit tests and docs
          Hide
          Petr Škoda added a comment -
          Show
          Petr Škoda added a comment - new unittest for concurrency added updated http://docs.moodle.org/en/Development:DML_functions#Delegated_transactions
          Hide
          Petr Škoda added a comment -

          committed detection of missing commit|rollback

          last todo:

          • print info in page footer, to be done later during theme improvements (we need some general debug notifications in footer)
          Show
          Petr Škoda added a comment - committed detection of missing commit|rollback last todo: print info in page footer, to be done later during theme improvements (we need some general debug notifications in footer)
          Hide
          Petr Škoda added a comment -

          the DB checker should already detect old uses of begin_sql)) and friends, right?

          Show
          Petr Škoda added a comment - the DB checker should already detect old uses of begin_sql)) and friends, right?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think it already looks for "begin_sql()" but not for ">begin_sql()"... adding it right now.

          Show
          Eloy Lafuente (stronk7) added a comment - I think it already looks for "begin_sql()" but not for ">begin_sql()"... adding it right now.
          Hide
          Petr Škoda added a comment -

          I guess this is finished, right?

          Show
          Petr Škoda added a comment - I guess this is finished, right?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm.. I think we are missing the environmental recommendation about to use InnoDB if running MySQL, you or me?

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm.. I think we are missing the environmental recommendation about to use InnoDB if running MySQL, you or me?
          Hide
          Petr Škoda added a comment -

          You, please

          Show
          Petr Škoda added a comment - You, please
          Hide
          Petr Škoda added a comment -

          We have the innodb warning and we switch to innodb by when installing and myisam is default.
          Thanks everybody.

          Show
          Petr Škoda added a comment - We have the innodb warning and we switch to innodb by when installing and myisam is default. Thanks everybody.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: