We have been adding transactions a lot lately, and we have just found that we are expecting a behaviour that is not the one we have. Take a look at the code posted on bug 26518 comment 7. If we turn auto_savepoint on, we will get the behaviour we expect. This patch has been initially attach on bug 21684 comment 2, but then dropped. See also https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#Nested-transactions-and-auto-savepoints
Created attachment 111388 [details] [review] Bug 26639: Add auto_savepoint to DBIC connection See comment 0. https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#Nested-transactions-and-auto-savepoints Otherwise: DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274 Possible side-effects? Slowness? We need to push it to master ASAP and see how our test suite behave.
I'm very much in favour of this. It means nested transactions, in general, will act as expected rather than the caller needing to have a deep knowledge of them and the underlying code to prevent errors. As for performance; In the little research I've done there is a reduction in performance by enabling this, but it's not huge.. there's actually more of a reduction in select speed performance with a trend of lower performance with each release of MySQL which is weird.. I think they've been favouring accuracy and concurrency over single threaded lookup speeds.
In fact, most frameworks appear to turn it on by default and you have to explicitly disable it these days.. DBIx::Class is old enough that MySQL didn't support it at the time (4.4 onwards support savepoints in innodb, so we're safe). Lets go for it.
Created attachment 111392 [details] [review] Bug 26639: Add auto_savepoint to DBIC connection See comment 0. https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#Nested-transactions-and-auto-savepoints Otherwise: DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274 Possible side-effects? Slowness? We need to push it to master ASAP and see how our test suite behave. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
QA: Will be testing here a bit ;)
Created attachment 111835 [details] [review] Bug 26639: Add auto_savepoint to DBIC connection See comment 0. https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#Nested-transactions-and-auto-savepoints Otherwise: DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274 Possible side-effects? Slowness? We need to push it to master ASAP and see how our test suite behave. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me. If we want developers to test this a bit more, we could do the same as we did with strict sql mode ? Enable it based upon config var.
Tested with a MariaDB container. Running Koha/Patrons.t (note that store contains a txn_do). This test creates 363 save points. Time for running the test was not really different. Tested also storing a patron 1000 times. The savepoints seem to be 5 to 7% slower. But since this is not a very common operation, this number cannot be attached to normal performance.
Thx, Marcel!
Pushed to master for 20.11, thanks to everybody involved!
Good news, I compared the different job execution times on Jenkins (before and after this patch) and I have not noticed any significant changes.
(In reply to Jonathan Druart from comment #11) > Good news, I compared the different job execution times on Jenkins (before > and after this patch) and I have not noticed any significant changes. That's great, Jonathan!
backported to 20.05.x for 20.05.06
backported to 19.11.x for 19.11.12
Backported to 19.05.x branch for 19.05.17