Bug 28534

Summary: pending_offline_circulations table uses MyISAM engine
Product: Koha Reporter: Nick Clemens <nick>
Component: DatabaseAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: major    
Priority: P5 - low CC: david, jonathan.druart, joonas.kylmala, kyle, martin.renvoize
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This updates the database structure for the pending_offline_operations table so that it uses the InnoDB engine instead of the MyISAM engine.
Version(s) released in:
21.11.00
Attachments: Bug 28534: Fix engine for pending_offline_circulations table
Bug 28534: Atomic update to fix existing installs
Bug 28534: Fix engine for pending_offline_circulations table
Bug 28534: Atomic update to fix existing installs
Bug 28534: Fix engine for pending_offline_circulations table
Bug 28534: Atomic update to fix existing installs
Bug 28534: (follow-up) Don't adjust original DB update
Bug 28534: Fix engine for pending_offline_circulations table
Bug 28534: Atomic update to fix existing installs
Bug 28534: (follow-up) Don't adjust original DB update
Bug 28534: Fix engine for pending_offline_circulations table

Description Nick Clemens 2021-06-09 13:44:30 UTC
This appears to be a holdover that was never updated
Comment 1 Nick Clemens 2021-06-09 18:12:48 UTC
Created attachment 121746 [details] [review]
Bug 28534: Fix engine for pending_offline_circulations table
Comment 2 David Nind 2021-06-10 19:44:37 UTC
I wasn't sure how to test this (using koha-testing-docker), but thought that this would work:

1. Check table structure in the database:
   . koha-mysql kohadev
   . show create table pending_offline_operations;
   . should show as "..ENGINE=MyISAM..."
2. Apply patch
3. Update the database (updatedatabase)
4. Check the table structure as per step 1 - should show as "..ENGINE=INNODB.."

It does update if I do a reset_all, but for the existing installations I seem to be missing something... (restarting the container didn't work for me - docker restart koha_db_1)?
Comment 3 Nick Clemens 2021-06-14 12:44:10 UTC
Created attachment 121908 [details] [review]
Bug 28534: Atomic update to fix existing installs
Comment 4 Nick Clemens 2021-06-14 12:47:00 UTC
(In reply to David Nind from comment #2)
> I wasn't sure how to test this (using koha-testing-docker), but thought that
> this would work:
> 
> 1. Check table structure in the database:
>    . koha-mysql kohadev
>    . show create table pending_offline_operations;
>    . should show as "..ENGINE=MyISAM..."
> 2. Apply patch
> 3. Update the database (updatedatabase)
> 4. Check the table structure as per step 1 - should show as
> "..ENGINE=INNODB.."
> 
> It does update if I do a reset_all, but for the existing installations I
> seem to be missing something... (restarting the container didn't work for me
> - docker restart koha_db_1)?

I added an atomic update to fix existing installs. I added a create table as well, one of our sites did not have MyISAM enabled, so the table was not created. To test:
1 - Check current table structure
2 - Apply patch
3 - Update database
4 - Confirm table structure is correct
5 - Drop the table 'DROP TABLE pending_offline_operations'
6 - Update database
7 - Confirm table is created
8 - reset_all
9 - Check the structure and confirm table is created as InnoDB
Comment 5 David Nind 2021-06-14 16:50:25 UTC
Created attachment 121917 [details] [review]
Bug 28534: Fix engine for pending_offline_circulations table

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2021-06-14 16:50:29 UTC
Created attachment 121918 [details] [review]
Bug 28534: Atomic update to fix existing installs

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Kyle M Hall 2021-07-23 13:36:19 UTC
Created attachment 123112 [details] [review]
Bug 28534: Fix engine for pending_offline_circulations table

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2021-07-23 13:36:30 UTC
Created attachment 123113 [details] [review]
Bug 28534: Atomic update to fix existing installs

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Jonathan Druart 2021-07-26 07:55:03 UTC
1. I would not modify the old DB rev

2.
There was a comment about that prior to
  commit 8c5845ef9ecd92584723cd60a221ecd4710a2516
  Bug 26947: Update kohastructure.sql


-- this table is MyISAM, InnoDB tables are growing only and this table is filled/emptied/filled/emptied...
-- so MyISAM is better in this case

So it seems that it was expected.

I don't know how it's still relevant however.
Comment 10 Nick Clemens 2021-09-14 13:06:16 UTC
Created attachment 124865 [details] [review]
Bug 28534: (follow-up) Don't adjust original DB update
Comment 11 Kyle M Hall 2021-09-14 13:24:45 UTC
> -- this table is MyISAM, InnoDB tables are growing only and this table is
> filled/emptied/filled/emptied...
> -- so MyISAM is better in this case
> 
> So it seems that it was expected.
> 
> I don't know how it's still relevant however.

This appears to be incorrect for the most part. It is only true if innodb_file_per_table is disabled, but innodb_file_per_table is enabled by default. It lists this behavior specifically as a reason not to disable it.

https://dev.mysql.com/doc/refman/8.0/en/innodb-file-per-table-tablespaces.html
Comment 12 Nick Clemens 2021-09-16 09:53:44 UTC
(In reply to Jonathan Druart from comment #9)
> 1. I would not modify the old DB rev
> 
> 2.
> There was a comment about that prior to
>   commit 8c5845ef9ecd92584723cd60a221ecd4710a2516
>   Bug 26947: Update kohastructure.sql
> 
> 
> -- this table is MyISAM, InnoDB tables are growing only and this table is
> filled/emptied/filled/emptied...
> -- so MyISAM is better in this case

tmp_holdsqueue is constantly filled/emptied and is InnoDB and has no issues

It seems the other problem of InnoDB is the resetting of auto_increment on a restart, which we have handled and should not affect here
Comment 13 Joonas Kylmälä 2021-09-19 13:20:58 UTC
So can we just obsolete those two patches? Let me know when this is ready for QA.
Comment 14 Kyle M Hall 2021-09-21 12:22:34 UTC
(In reply to Joonas Kylmälä from comment #13)
> So can we just obsolete those two patches? Let me know when this is ready
> for QA.

I think all three are needed. The first one fixes the koha structure file, the second one fixes existing installs, and the third one reverts the changes in the first patch to the original db update that creates the table.
Comment 15 Martin Renvoize 2021-09-24 15:20:38 UTC
Created attachment 125265 [details] [review]
Bug 28534: Fix engine for pending_offline_circulations table

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-09-24 15:20:43 UTC
Created attachment 125266 [details] [review]
Bug 28534: Atomic update to fix existing installs

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-09-24 15:20:47 UTC
Created attachment 125267 [details] [review]
Bug 28534: (follow-up) Don't adjust original DB update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-09-24 15:21:10 UTC
Nice work, Passing QA
Comment 19 Jonathan Druart 2021-09-28 10:32:28 UTC
Created attachment 125393 [details] [review]
Bug 28534: Fix engine for pending_offline_circulations table

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 28534: (follow-up) Don't adjust original DB update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Jonathan Druart 2021-09-28 13:38:24 UTC
Pushed to master for 21.11, thanks to everybody involved!