Bug 26639 - Turn auto_savepoint ON
Summary: Turn auto_savepoint ON
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 26518
  Show dependency treegraph
 
Reported: 2020-10-09 09:59 UTC by Jonathan Druart
Modified: 2020-11-17 13:18 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06, 19.11.12, 19.05.17


Attachments
Bug 26639: Add auto_savepoint to DBIC connection (1.09 KB, patch)
2020-10-09 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26639: Add auto_savepoint to DBIC connection (1.16 KB, patch)
2020-10-09 10:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26639: Add auto_savepoint to DBIC connection (1.25 KB, patch)
2020-10-16 09:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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