• 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.

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

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.

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

$DB->transaction_wrapper(

);

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.

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.

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

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.

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.

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.

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

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?

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.

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.

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

thanks, Moodle community rocks :-D

• 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.

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?

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

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

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

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

$tr->abort_transaction($ex);

which would automatically rethrow the exception

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.

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

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.

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.

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

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.

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.

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.

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.

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

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.

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).

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

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)

+1 for transaction_required / forbidden

what does transaction_allowed do?

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

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

$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.

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

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()

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

I hope it fulfils all requirements

ciao

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

going to work more on unit tests and docs

• new unittest for concurrency added
• updated http://docs.moodle.org/en/Development:DML_functions#Delegated_transactions

last todo:

• print info in page footer, to be done later during theme improvements (we need some general debug notifications in footer)