Bug 26639

Summary: Turn auto_savepoint ON
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: 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
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
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
Comment 1 Jonathan Druart 2020-10-09 10:01:19 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.
Comment 2 Martin Renvoize 2020-10-09 10:13:06 UTC
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.
Comment 3 Martin Renvoize 2020-10-09 10:15:53 UTC
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.
Comment 4 Martin Renvoize 2020-10-09 10:38:03 UTC
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>
Comment 5 Marcel de Rooy 2020-10-16 06:31:59 UTC
QA: Will be testing here a bit ;)
Comment 6 Marcel de Rooy 2020-10-16 09:23:51 UTC
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>
Comment 7 Marcel de Rooy 2020-10-16 09:25:18 UTC
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.
Comment 8 Marcel de Rooy 2020-10-16 09:32:24 UTC
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.
Comment 9 Katrin Fischer 2020-10-16 10:04:52 UTC
Thx, Marcel!
Comment 10 Jonathan Druart 2020-11-02 14:56:08 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Jonathan Druart 2020-11-04 08:13:18 UTC
Good news, I compared the different job execution times on Jenkins (before and after this patch) and I have not noticed any significant changes.
Comment 12 Tomás Cohen Arazi 2020-11-04 12:12:23 UTC
(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!
Comment 13 Lucas Gass 2020-11-16 18:52:22 UTC
backported to 20.05.x for 20.05.06
Comment 14 Aleisha Amohia 2020-11-17 05:18:04 UTC
backported to 19.11.x for 19.11.12
Comment 15 Victor Grousset/tuxayo 2020-11-17 13:18:59 UTC
Backported to 19.05.x branch for 19.05.17