Bug 18364 - LOCK and UNLOCK are not transaction-safe
Summary: LOCK and UNLOCK are not transaction-safe
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15854
Blocks: 17964
  Show dependency treegraph
 
Reported: 2017-03-31 16:38 UTC by Jonathan Druart
Modified: 2020-03-30 08:27 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18364: Do not LOCK/UNLOCK tables from tests (2.89 KB, patch)
2017-03-31 16:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18364: Do not LOCK/UNLOCK tables from tests (2.94 KB, patch)
2017-04-12 10:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18364: Do not LOCK/UNLOCK tables from tests (3.04 KB, patch)
2017-04-20 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18364: [Follow-up] Also add an environment variable to prevent locking (2.50 KB, patch)
2017-04-20 11:28 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 2017-03-31 16:38:43 UTC
From the MySQL doc:
"LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables."
Comment 1 Jonathan Druart 2017-03-31 16:48:21 UTC
Created attachment 61758 [details] [review]
Bug 18364: Do not LOCK/UNLOCK tables from tests

From the MySQL doc:
"LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables."
If the LOCK/UNLOCK statements are executed from tests, the current transaction will be committed.
To avoid that we need to guess if this code is execute from testsa or not (yes it is a bit hacky)

Better ideas are welcome!

Another fix would have been to revert
  commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad
  Bug 15854: Use a READ and WRITE LOCK on message_queue
but theorically a race is still possible.

Existing tests seem to be safe, to test this patch you will need new
tests from bug 17964.
Test plan:
  prove t/db_dependent/Letters/TemplateToolkit.t
twice, and notice that changes have been comitted.
Comment 2 Marcel de Rooy 2017-04-01 06:45:21 UTC
"yes it is a bit hacky"
Not a bit imo LOL
Comment 3 Julian Maurice 2017-04-03 07:03:30 UTC
If LOCK TABLE commits any active transaction, is it useful to start a transaction just before (line 3385) ?

> Better ideas are welcome!

While it may be a change too big for this patch, has someone already considered using a separate database for tests ?
Comment 4 Marcel de Rooy 2017-04-03 07:31:26 UTC
(In reply to Julian Maurice from comment #3)
> If LOCK TABLE commits any active transaction, is it useful to start a
> transaction just before (line 3385) ?

Would it solve the problem? Can imagine that LOCK does not like a nested transaction either.
Comment 5 Jonathan Druart 2017-04-03 13:28:39 UTC
(In reply to Julian Maurice from comment #3)
> If LOCK TABLE commits any active transaction, is it useful to start a
> transaction just before (line 3385) ?

No I do not think so, I wanted to remove it in this patch but did not want to add more confusions/complexity.
Comment 6 Jonathan Druart 2017-04-03 13:29:25 UTC
The doc says

"""
The correct way to use LOCK TABLES and UNLOCK TABLES with transactional tables, such as InnoDB tables, is to begin a transaction with SET autocommit = 0 (not START TRANSACTION) followed by LOCK TABLES, and to not call UNLOCK TABLES until you commit the transaction explicitly.
"""

https://dev.mysql.com/doc/refman/5.7/en/lock-tables-and-transactions.html
Comment 7 Nick Clemens 2017-04-12 10:19:56 UTC
Created attachment 62074 [details] [review]
Bug 18364: Do not LOCK/UNLOCK tables from tests

From the MySQL doc:
"LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables."
If the LOCK/UNLOCK statements are executed from tests, the current transaction will be committed.
To avoid that we need to guess if this code is execute from testsa or not (yes it is a bit hacky)

Better ideas are welcome!

Another fix would have been to revert
  commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad
  Bug 15854: Use a READ and WRITE LOCK on message_queue
but theorically a race is still possible.

Existing tests seem to be safe, to test this patch you will need new
tests from bug 17964.
Test plan:
  prove t/db_dependent/Letters/TemplateToolkit.t
twice, and notice that changes have been comitted.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Marcel de Rooy 2017-04-12 11:15:49 UTC
$ENV{_} =~ m|prove|;

I'd still rather see a specific environment variable. As mentioned on irc earlier, we could set some kind of lock variable in the environment via a shared apache file or cron file. A test would not see that var and not lock the table.

Another option (with some more impact on the code) would be to switch from table locking to some kind of intelligent file locking here. (We use file locking in rebuild zebra, but it should be more refined.) Advantage would be that we do not differentiate between calls from a unit test and from other scripts.

And the current test does not include running a separate test file from the command line without prove (which I normally do). Could we test on a .t suffix too?

In conclusion, I would not favor pushing this in its current from. Other opinions are welcome.
Comment 9 Marcel de Rooy 2017-04-12 11:23:07 UTC
(In reply to Jonathan Druart from comment #6)

> The correct way to use LOCK TABLES and UNLOCK TABLES with transactional
> tables, such as InnoDB tables, is to begin a transaction with SET autocommit
> = 0 (not START TRANSACTION) followed by LOCK TABLES, and to not call UNLOCK
> TABLES until you commit the transaction explicitly.

Did you consider record locking instead of table locks here? Not sure if it would resolve the problem..
Comment 10 Jonathan Druart 2017-04-12 13:33:51 UTC
(In reply to Marcel de Rooy from comment #9)
> (In reply to Jonathan Druart from comment #6)
> 
> > The correct way to use LOCK TABLES and UNLOCK TABLES with transactional
> > tables, such as InnoDB tables, is to begin a transaction with SET autocommit
> > = 0 (not START TRANSACTION) followed by LOCK TABLES, and to not call UNLOCK
> > TABLES until you commit the transaction explicitly.
> 
> Did you consider record locking instead of table locks here? Not sure if it
> would resolve the problem..

Do you mean 'SELECT FOR UPDATE'?
I tried it yes, see commit message for bug 15854.
Comment 11 Marcel de Rooy 2017-04-20 11:27:56 UTC
Created attachment 62458 [details] [review]
Bug 18364: Do not LOCK/UNLOCK tables from tests

From the MySQL doc:
"LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables."
If the LOCK/UNLOCK statements are executed from tests, the current transaction will be committed.
To avoid that we need to guess if this code is execute from testsa or not (yes it is a bit hacky)

Better ideas are welcome!

Another fix would have been to revert
  commit be156d9ad9e5bcfadab34d44f90e04fd61e256ad
  Bug 15854: Use a READ and WRITE LOCK on message_queue
but theorically a race is still possible.

Existing tests seem to be safe, to test this patch you will need new
tests from bug 17964.
Test plan:
  prove t/db_dependent/Letters/TemplateToolkit.t
twice, and notice that changes have been comitted.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-04-20 11:28:01 UTC
Created attachment 62459 [details] [review]
Bug 18364: [Follow-up] Also add an environment variable to prevent locking

The test in SendCirculationAlert is extended by adding an env var
called KOHA_NO_TABLE_LOCKS. If this var is set to a true value,
the table locking is skipped too.

This is useful when running a test without prove. The variable could be
set in a shell profile.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Kyle M Hall 2017-04-21 11:35:51 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 14 Katrin Fischer 2017-04-21 22:20:16 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.
Comment 15 Mason James 2017-05-03 03:49:04 UTC
Pushed to 16.05.x, for 16.05.12 release
Comment 16 Mason James 2017-05-03 03:50:52 UTC
i think 3.22.x needs this patch, because of BZ-15854
Comment 17 Julian Maurice 2017-05-10 09:26:48 UTC
Pushed to 3.22.x for 3.22.21
Comment 18 Lari Taskula 2017-07-27 12:01:34 UTC
(In reply to Marcel de Rooy from comment #8)
> And the current test does not include running a separate test file from the
> command line without prove (which I normally do). Could we test on a .t
> suffix too?

Sorry to jump into this Bug after pushing, but I didn't see an answer for this idea. I see you use an environment variable but testing on .t suffix sounds like a nice solution when not using prove and being unaware of the environment variable. Did you discuss it on IRC?
Comment 19 Marcel de Rooy 2017-07-27 12:27:32 UTC
(In reply to Lari Taskula from comment #18)
> (In reply to Marcel de Rooy from comment #8)
> > And the current test does not include running a separate test file from the
> > command line without prove (which I normally do). Could we test on a .t
> > suffix too?
> 
> Sorry to jump into this Bug after pushing, but I didn't see an answer for
> this idea. I see you use an environment variable but testing on .t suffix
> sounds like a nice solution when not using prove and being unaware of the
> environment variable. Did you discuss it on IRC?

Yes, it probably was. I recall that testing on the .t suffix was not that easy after all, although it sounds so..
Comment 20 Jonathan Druart 2020-03-30 08:27:34 UTC
KOHA_NO_TABLE_LOCKS is going to be renamed KOHA_TESTING by bug 25018.