here is a test https://github.com/skodak/moodle/tree/w39_MDL-35506_m24_pgerrorrollback

This might probably bork our PHPUnit integration too in some weird ways.

I am not really sure this should be considered for backporting to stable - it may affect performance and it needs testing on real sites, the phpunit test works, but I never used savepoints before...

If the only report revealing this until now was MDL-32572 and it has been fixed in stables by taking rid of such a mammoth transaction... I think it's ok to apply this only to master, yup.

Patch looks ok. Let's see the impact of such extra savepoints on current transactions.

Ciao

unittest execution before 1:27, after 1:36

Hi Petr,

I don't understand from the issue description what the problem is or the desired behaviour is? Should we be rolling back, shouldn't we? Its not clear from the issue.

Dan, see the unit test. The point is to have consistent error handling in transactions for all supported databases.

I understand your point about consistency across database, but i'm still lost.

What is not clear to me is what the desired behaviour is. The previous pg behaviour seems OK to me?? This change seems to roll back only one query deep? In which case, what is the point of using the transaction?? The unit test doesn't demonstrate to me what the expected transaction behaviour is.

I'm clearly missing something..

The expected behaviour is:
If there is a database error it should always only trigger exception. If inside transaction the triggered exception should rollback everything only if it is not handled.

Without this patch it is impossible to try-catch database exceptions inside a transaction in pgsql because the database connection is automatically rolled back by postgresql server, not our default exception handler, you can not recover from a bad insert for example.

Aha, thanks.

So could I change this issue summary to "Postgres rolls back transaction even if exception is caught"?

Note: This is different to what I thought the behaviour was, I thought that the transaction handling was entirely DB handled. So others might not understand the distinction either.

Integrated, thanks.

Hmm. Though... Isn't this trippling the round trips to the server? We need to test this on a remote server to measure the performance impact.

Yes, the performance is lower when transaction is in progress. Unfortunately requests with bound parameter can not contain multiple queries separated by semicolons.

Adding Tony Levi from Netspot & Sam M from the OU as two potential big sites which might be able to offer insight into this.

I'd be concerned about this change if it worsens performance (triples round trips, + presumably postgres has to do some more effort for those save states?) - although transactions are probably mainly used only on 'write' requests such as posting a forum(/NG) message, that could still have a performance impact.

We seem to manage fine with the current behaviour. I understand this as 'how it works' and have had no real problems dealing with it, except as something you have to bear in mind.

At the time when database transactions were introduced, I'm sure I remember that conceptually we/Eloy decided that you are not allowed to catch those exceptions, presumably for exactly this reason - that is, you can catch them to display a nicer error or something, but you must still not attempt to do anything else in the database in that request.

If we need to ensure consistent handling across databases without reducing performance, what is the prospect of taking the other approach by enforcing what I just said - could we make it so that on other databases, when an exception is thrown inside a transaction, it automatically does rollback at that point or something similar, to emulate the Postgres behaviour?

Alternatively there could be something like an option parameter to start_deferred_transaction or whatever that function is called, where you can pass 'true' to indicate that you intend to catch exceptions and therefore need to enable weird crap (tripling round-trips) in databases that wouldn't otherwise support that? Since it is clearly not needed in most parts of the page.

We do not "seem to manage fine", see the linked issue where it was incorrectly rolling back transactions. I did not find any other backwards compatible approach, sorry, if we decide to rollback everything on first database exception in all drivers then it is a big change that should be imo done at the start of development cycle.

My +1 to not change the DML API two weeks before the 2.4dev freeze.

My +1 to not severely impact performance on the most used database for the largest (and consequently, slowest) installations.

Which test exactly is showing a problem? I think this is just bad code not using transactions right.

Further to the above, I would point out:

http://docs.moodle.org/dev/DB_layer_2.0_delegated_transactions

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

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

It appears this is working exactly as intended and any deviation from this documentation is clearly an API change.

BTW, I have no problem with adding savepoints support to DML in general; it has been useful to me for a few specific cases. But it needs to be an optional explicit call from some code as-needed, not implied for each query just by being in a transaction.

As far as I can tell, the behaviour works entirely to spec, so is this really a blocker?

I'm setting this to 'testing failed' (though the unit tests work).

The line which Tony has found is really key to me. It sounds to me like Postgres is working exactly to that spec and the other dbs are not. So, we need to discuss this, particularly with the potential performance impact.

Spec is not the ultimate authority, it can be fixed easily to match the reality. Both Eloy and me (author of the spec and code) were surprised by this behaviour, it seems that other developers did not expect it either. Clearly the DML API was designed to not rollback automatically on error - trust me.

My patch does not require any other changes in code. If we decide to rollback always on any database exception then all code using exceptions needs to be reviewed and fixed.

Just commenting that the psql client has an option - ON_ERROR_ROLLBACK which does what we are achieving here. But Petr/Eloy were not able to find a way to implicitly set that. http://www.postgresql.org/docs/8.3/static/app-psql.html

Summary (from a recent chat at integrators room):

if PG sucks... sucks guys. When others have sucked... we have had to adapt them to the ANSI behavior. This is the same, 100% the same.

One possible solution:

Let's assume that 99% of the times... we are using transactions to execute SQL statements together in order to rollback them together (that's - basically - their primary utility, agree).

What about allow transactions... when created... to simply determine if the complete rollback is the expected behavior and allow it to happen?

Something like:

public function start_delegated_transaction($autorollback = false)

So, default will be current behavior (don't assume we want rollback), but we'll allow to specify if we want autorollback.

Just one quick idea. For sure it needs to be analyzed because, perhaps, in order to allow that change we need to make all the rest of drivers slower.... so consider it only as one "ideal" idea.

Finally, don't forget that:

WE CANNOT RELY ON TRANSACTIONS within Moodle core !!!

(until we mandatorily enforce all DBs to have them working. We aren't right now - the MySQL ISAM case).

Ciao

Hmm, I think I could significantly lower the number of roundtrips by snapshotting only at the start of transaction and after all write operations. That should eliminate most of the perf problems I guess and it would make the transactions easy to use and predictable.

I'm clearer now so sending this back to testing.

What I think we need, is a run against a remote db server (i.e. not running on same machine as developer) to try and guage the impact.

Running the phpunit tests probably induces more transactions than core actually uses (git grep start_delegated_transaction)

I have pushed two more commits there - it is now adding only 1 query to update queries, it also includes more tests for all query types.

I've integrated the new commits. I've also look at the impact on the nightly build perf stats and well, its had none. (That is a lot to do with the lack of transactions in almost all Moodle pages).

I've run the tests on pg, oracle and mysql, looking good.

On my machines the tests are also not really imapcted on time. Would be good to have arun against a non-local db server.

Eloy: I'm surprised you are surprised by this because I know it came up in initial discussion about exceptions in DDL, it was definitely discussed at the point which is why somebody wrote that sentence in the spec. The existing API definitely included this assumption when originally designed. Continuing to enforce it would not be a change to the spec.

We have load test infrastructure and I think we are planning another round of load testing soon in order to compare 2.2 and 2.3, as we gear up to launch 2.3 here in December. Our load tests are based on our actual usage mix, so there's definitely a test that covers ForumNG (which uses transactions for all the 'post' actions, since they involve more than one write query) which would be a good test.

I'm not sure who is doing that, I think it may be Tim, and I'm also not sure what the timescale is. But hopefully it would be possible to take the ForumNG test and try it on standard 2.3 vs 2.3 with this patch applied. This would give an actual test, at significant load, with database on separate server, running code that actually uses transactions for its 'write' requests which are triggered in a reasonable proportion - i.e. as realistic a test as we could have of the situation that would be affected by any performance problems this change might or might not cause.

I'll find out who is doing that and when to see if this can fit in. Of course, if there is another heavy Moodle user geared up to do similar load testing, maybe they could try that kind of thing too.

After doing realistic load testing, if there is no performance decrease then I don't see any problem with the change. If there is a performance decrease then I like Eloy's suggestion about adding a parameter (that defaults to false) that indicates whether you intend to abuse/catch database exceptions within the transaction. Databases can then use this as a hint to do nothing (most databases) or do extra stuff (Postgres), same as the other hints we have in various places.

I talked to Rod who is probably going to be setting up our 2.3 load testing, we hope in early October. It should hopefully be feasible to do an extra run of the ForumNG test with this patch applied so that we can compare it to the existing situation and see if performance is worse.

Regarding the performance impact, initially I was concerned. But, i've looked more at this from a core POV and I really can't see it even being a blip in the ocean compared to the other problems we have or will face. Furthermore, the improvements that have resulted from this discussion make the impact of this even less. (yay, discussion = better code, please continue this).

My thoughts on this now is that i'd still love for some real performance metrics to be sure, but we have to take in account that some silly programmer (call him Dan P) can (and does, i'm afraid) introduce code which might causes more of an effect than this theoretical problem.

But all that said.
1) Its great to have big moodle sites aware of this problem. Maybe if we're all totally wrong you can alert us early. Please!
2) Surely this discussion shows that Eloy/Petr, but not to all of us. I was not clear on the expected behaviour and the discussion shows others too. Even though we are not depending on transaction support in core, we might be missing things.

(added stronk7 as a watcher to this issue, even though I know he gets too much email)

I am really surprised that such a fundamental change has been proposed, developed and integrated with absolutely no discussion in any Moodle forum, or in developer's chat, and no load testing done.

Can I suggest that we:

1. Revert this change this week.
2. Petr starts a thread in http://moodle.org/mod/forum/view.php?id=45 explaining the proposed change.
3. Someone does some load testing.
4. Then, if all is well, this gets integrated shortly before the 2.4 code freeze.
5. A new bit is added to http://docs.moodle.org/dev/Integration_Review ... actually Integration Review Process step 5 "Check the right people have been involved" should have caught this, but for clarity you could add a bit "for fundamental changes, check that a thread has been started in an appropriate forum, and other Moodlers given enough time to comment."

Dan: I agree the current situation is not a good one because the original design was difficult to understand let alone communicate. Doing nothing is not a good solution to this problem - either we should make this change, or else enforce the original design (make it so you definitely can't continue after a db exception, whatever database you're using), or a mixture of both (by having a function that indicates whether you are going to catch the exceptions or not).

We can't know which of those three is the right decision unless we have some credible performance data. (I wonder if anyone else has load testing for a workload that includes a reasonable amount of transactions?)

Tim: the point is that this isn't a fundamental change in behaviour.

In fact, all DB drivers other than Pg are currently passing the unit tests Petr has created, apart from Pg. So we are only making the PG drivers conform to a standard, like the other dbs. This is about making PG observe the ANSI SQL Transaction standard (as I understand it).

I agree these is confusion here, but really your suggestion that Petr posts a forum post saying 'I'd like to propose postgres works exactly as the other supported drivers' is ridiculous?!

Let me repeat what this patch does - it makes the transactions consistent in all 4 supported databases. This patch does not break any backwards compatibility, if you rolled back manually in try-catch before it is still going to work. PostgreSQL seems to be conflicting with other databases and ANSI SQL standard here, sorry.

Yes, performance has lower priority because the most important aspect of DML layer is the consistency and predictability. The original spec was just a wish list, it is not a documentation of how I and Eloy actually implemented the transactions in the DML layer or how I implemented the exceptions support in Moodle.

This was committed only to 2.4dev, it is the responsibility of others to test the release previews with real life data and report any regressions including performance. I do not have access to any production data, sorry, I wrote unit tests and tested what I could. If there really is a need to implement fast-pg transactions with automatic rollback then it needs to be done for all supported databases and somebody has to pinpoint the critical transactions and use it there.

I was already bitten by this PostgreSQL behaviour a few times before (webservices and phpunit support for example), I just did not realise/understand it at that time. The worst thing is that it goes unnoticed if you use non-pg database and it may result in real data loss on PostgreSQL.

(would we be having this discussion if it was oracle, not Postgres, which was the odd one out)

Note: I do feel responsible for any regressions this patch may create, please test it and report any problems. Previously the feedback was usually very delayed which made any fixing problematic. I think the optional auto-rollback-on-error would not be very difficult to implement, if somebody proves the current patch causes perf trouble it should take a few days only to create it - the usage might be a bit confusing for developers though and it would not be backwards compatible with current code, you'd need to be very carful what you are catching, we might have to change some exception from DML layer too and you would need to watch out for double transaction rollbacks (new error messages probably in DML layer).

Off-topic: RE: 'A new bit is added to http://docs.moodle.org/dev/Integration_Review', I agree and have added it (top level) as I think the point is a good one.

Thanks for explaining.

I (think I) remember the original discussion about how transactions should work, like sam does, and I think Pg is currently working the way we thought transactions would work. However, if as you say, all other DBs are working the other way, and that does not cause problems, then good.

I still think it would have been good practice to post in the DB forum before now. Of course anyone could do that, so I can do it myself, and have.

Can I ask that you update http://docs.moodle.org/dev/DB_layer_2.0_delegated_transactions to match the expected reality if this change is going to go through, otherwise it'll just lead to more confusion as to the behavior when PG suddenly stops meeting that specification

Dan wrote: "This patch does not break any backwards compatibility"

This is of course complete nonsense - anybody following the documented API is expecting the current PG behavior to happen. The proposed patch fundamentally changes behavior and is clearly a BC break.

The tests are wrong and the other DBs are wrong.

Agreed that is a misleading statement.

I'm sure this debate will go on.

But i've tested this on all supported DBs and after the change the behaviour in now consistent across all the DBs, so i'm passing this test.

I did ask in the postgres irc channel to check if there was a way to do this with the libraries and there is not.

I've added the dev_docs_required flag for the docs change. I've also added api_change so that this gets noted in the release notes.

The test is wrong as it does not conform to the historical and documented API.

Can one really commit any arbitrary and broken unit test now and change core API?

Tony: No, but behaviour inconsistency between the dbs is a bug.

When the original authors/visionaries of the API say that the API should have always been behaving like X and all the databases but PG are behaving like X, making the behaviour conform to X is a reasonable, low risk[1] way forward, despite our docs sucking.

[1] Its not like we're short of a mysql install base.

The problem is all the code that depends on the documented behaviour - we will now have to audit a bazillion integrations and make some kind of core hack to get proper transactions. If you're going to break the API, you at least need to at least offer a way to get the same functionality.

I disagree, this change does not break backwards compatibility, previously you had to rollback the transaction ALWAYS on SQL error in PostgreSQL (if you did not the pgsql driver kept complaining and you could not do anything else). Now you can decide if you want to continue or not with the transaction after error like in all other databases.

Tony, could you please expose where the BC breakage is happening with this change. I'm really sorry, but I don't get it at all.

"proper transactions" is what all DBs do by default, but PostgreSQL. With the change, PostgreSQL meets expected/ansi behavior.

You can discuss if that behavior is the "most used/useful" or no. Of you can discuss if 99% of the times, automatic rollback on error is the logic way (I think it is, btw, else the transaction is really poorly planned IMO - generally speaking).

But this is only about to make all DBs perform the same way (proper/ansi). And now you can decide if you want rollback to happen (default behavior, not changed) or no (not working previously under PostgreSQL at all).

So, where is the point of BC breakage? If you were assuming PG behavior was the correct (auto-rollback), that has not changed. Only if you catch the error, you'll need to manually rollback/rethrow.

Not to say, again, that relying on transactions is not recommended at all, until we have all DBs strictly supporting transactions. I won't stop saying that until we take rid of mysql ISAM (btw, Sam, don't get the "surprised" comment above).

Ciao

Eloy - about the 'surprised' comment - somebody gave the impression that you were not aware of this issue with how Postgres behaved, or how it had been written into the spec. But I'm pretty sure you were because when transactions were already discussed, several of us had a conversation specifically about this issue and everybody agreed that we would simply have the restriction that you basically aren't allowed to catch db exceptions. Which is why it got written in the doc and everything. Of course you may have forgotten, or maybe you really weren't in that discussion way back when, but I thought you were...

Regarding ANSI compliance I don't think this is relevant, it is up to Moodle data layer to decide how it behaves & it can place additional restrictions on top of ANSI SQL transactions if it likes (as was previously the case, until this change).

About the rest: I don't think this change causes a backward compatibility problem, in that I can't see how any code will stop working as a result. But it might cause a performance regression, we simply don't know. We will try to evaluate the performance of this patch as best we can, hopefully in early October as noted. If it doesn't have a performance impact (when using code which uses transactions) then fine. If it does then there may be a need to reconsider.

Thanks Sam! If there is a performance problem we can implement a new option for automatic rollback on error which would be consistent in all supported databases. If nobody proves it is significantly slower on large installs then my -1 for this new option.

Congratulations, you've done it!

Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

Well, I think the backwards compatibility problem is if people are catching non-specific exceptions? Trying to come up with a non-contrived example is difficult, but something like this:

<?php

$DB->start_delegated_transaction();

foreach($user){
   try{
   		sync_user_with_external_system($user);
   }catch (exception e){
   		// can't connect to external system, exit loop and wait until later
   }
	
}

$DB->allow_commit();

function sync_user_with_external_system($user) {
    if (!connect_to_external()){
		throw new external_service_exception('cant connect');
	}
	// do stuff
	
	$DB->execute('UPDATE USER...');
}

Sam - 100% - 100% - 100% (I'm 100% sure you are 100% correct about this issue leading to the "don't allow to catch approach". But believe me, I've 100% forgotten it since then (and I took this as a nasty "surprise").

BTW... we really need to change / annotate http://docs.moodle.org/dev/DB_layer_2.0_delegated_transactions (that page is only the plans 3-4 years ago IMO) and point to proper documentation page http://docs.moodle.org/dev/Data_manipulation_API#Delegated_transactions and examples.

Just to update on this, Rod has now done the load testing for this change (comparing the OU version of 2.3.x release, with and without this patch applied). It does make performance slightly worse, but not enough to concern us, so we are not going to object to this change.

I have requested the actual numbers for comparison, and will add another comment once I get them.

Big thanks for the info!

Here are the details. Test 1 is using the plain 2.3.x code; Test 2 is after applying the patch. This test is supposed to be run with a similar amount of load to our live system (but scaled because the test uses fewer servers) for a short period. Our test platform is actually slower than the live system, which is why these times aren't very good.

Request type      Test 1  Test 2 % difference
VL2ForumNG_View   1.163   1.248	 7%
VL2Subpage_View   0.987   1.108  11%
VL2OUContent_View 0.979   1.149  15%
VL2Course_View    1.003   1.247  20%
VL2ForumNG_Ajax   1.345   1.493  10%
VL2ForumNG_Post   1.345   1.493  10%

Not quite sure how statistically significant these are, but it looks like about 10% slower. So, fairly serious but not out of the ordinary for a Moodle release. If there are other changes which improve performance, they could perhaps cancel this out.

The 20% for course view is quite alarming if the only change between the two was this patch. That's one of moodles most hit pages.

Seriously alarming guys - didn't learn anything from the context rewrite performance disaster?

This needs a different solution.

Thanks Sam. Is OU using transactions a lot in own code that is not part of the core? Was it the OU fork or vanilla 2.3.x with OU data only?

I have created new issue MDL-36182 for new parameter that would allow developer to decide which type of on-error behavior to use. I am afraid it will take some time to get this implemented and tested.

Thank you OU for following through with measurements.

I'm intrigued as to why this is so high on the course view page as I don't recall any transactions in use on the course view page.

Overview block stuff from forumng ?

Hi, some responses:

1) All measurements were from OU Moodle 2.3 - we have very few core changes in 2.3 (none that should affect performance), but we do have oodles of custom plugins. We can't run tests against core Moodle 2.3 because this is using copies of our live data which is all based on our plugins.

2) Regarding the course view, I don't think this should be taken as a huge cause for concern compared to the others because although the course view page is a standard page, we have a custom course format and custom blocks so it is not really any more standard than any others here. (We also turned off many of the standard blocks.) By the way, ForumNG does make a DB request on the course view page (to check unread posts) but this does not use a transaction.

3) Regarding popularity of these pages, we actually chose the requests that are most popular on our live system (& are measured in our performance tracking system) to select the things tested, so yes all of these are very popular requests.

4) I was also surprised to see that the decrease was so uniform, because for example, although ForumNG does use transactions whenever it saves stuff that requires more than one query, when you just look at a page, I would have thought (have not checked in detail) it doesn't use transactions. However, bear in mind this was a test simulating realistic but high load, so we sent many requests in parallel - if the requests that DO use transactions put a slight extra load on the database, that might cause small delays for the ones that don't.

5) Possibly we do use transactions in some places even when doing read-only requests, this is something we could track by displaying something and looking at pages, if we wanted to optimise performance.

As I understand it, the plan is that the cacheing system in 2.4 should stop it doing those annoying table-structure checks it does on Postgres, and some other requests, which I would have thought should be enough to get back the performance lost here. Unfortunately, we won't be in a position to run performance tests against 2.4 to verify this until the necessary work has been done to include it in the OU's version, which won't be until well after it comes out. (Obviously, since we're doing our 2.3 tests now.)

If it is uniform and affects course view too this suggests the DB is under higher total load thanks to the writes and that has affected everything. Bad news for anybody interested in ForumNG.

Yes it would be better if this performance decrease could be avoided (for example by adding a parameter to start_transaction that indicates whether or not you are allowed to catch exceptions within the transaction).

I think Moodle core is actually still interested in ForumNG. (And I sincerely hope that whatever replacement or enhancement to existing forum is adopted, it will use transactions to ensure consistent state, on databases that support this.)