Summary: | Turn auto_savepoint ON | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | critical | ||
Priority: | P5 - low | CC: | aleisha, dcook, kyle, lucas, m.de.rooy, martin.renvoize, nick, tomascohen, victor |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35462 |
||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
20.11.00, 20.05.06, 19.11.12, 19.05.17
|
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 26518, 27707 | ||
Attachments: |
Bug 26639: Add auto_savepoint to DBIC connection
Bug 26639: Add auto_savepoint to DBIC connection Bug 26639: Add auto_savepoint to DBIC connection |
Description
Jonathan Druart
2020-10-09 09:59:35 UTC
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 |