[Bug 18364] New: LOCK and UNLOCK are not transaction-safe
From the MySQL doc: "LOCK TABLES is not transaction-safe and implicitly commits any active
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Bug ID: 18364 Summary: LOCK and UNLOCK are not transaction-safe Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org transaction before attempting to lock the tables." -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15854 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15854 [Bug 15854] Race condition for sending renewal/check-in notices -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17964 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17964 [Bug 17964] TT syntax for notices - Prove that CHECKIN and CHECKOUT are compatible -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
From the MySQL doc: "LOCK TABLES is not transaction-safe and implicitly commits any active
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 61758 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=61758&action=edit Bug 18364: Do not LOCK/UNLOCK tables from tests 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- "yes it is a bit hacky" Not a bit imo LOL -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
From the MySQL doc: "LOCK TABLES is not transaction-safe and implicitly commits any active
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #61758|0 |1 is obsolete| | --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 62074 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62074&action=edit Bug 18364: Do not LOCK/UNLOCK tables from tests 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- $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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
From the MySQL doc: "LOCK TABLES is not transaction-safe and implicitly commits any active
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62074|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 62458 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62458&action=edit Bug 18364: Do not LOCK/UNLOCK tables from tests 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 62459 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62459&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 17.05, thanks Jonathan! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de Status|Pushed to Master |Pushed to Stable --- Comment #14 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- These patches have been pushed to 16.11.x and will be in 16.11.07. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com --- Comment #15 from Mason James <mtj@kohaaloha.com> --- Pushed to 16.05.x, for 16.05.12 release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_3_22_candidate --- Comment #16 from Mason James <mtj@kohaaloha.com> --- i think 3.22.x needs this patch, because of BZ-15854 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- Pushed to 3.22.x for 3.22.21 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Lari Taskula <lari.taskula@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lari.taskula@jns.fi --- Comment #18 from Lari Taskula <lari.taskula@jns.fi> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18364 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|Oldversions |--- Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25018 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- KOHA_NO_TABLE_LOCKS is going to be renamed KOHA_TESTING by bug 25018. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org