This appears to be a holdover that was never updated
Created attachment 121746 [details] [review] Bug 28534: Fix engine for pending_offline_circulations table
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)?
Created attachment 121908 [details] [review] Bug 28534: Atomic update to fix existing installs
(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
Created attachment 121917 [details] [review] Bug 28534: Fix engine for pending_offline_circulations table Signed-off-by: David Nind <david@davidnind.com>
Created attachment 121918 [details] [review] Bug 28534: Atomic update to fix existing installs Signed-off-by: David Nind <david@davidnind.com>
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>
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>
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.
Created attachment 124865 [details] [review] Bug 28534: (follow-up) Don't adjust original DB update
> -- 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
(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
So can we just obsolete those two patches? Let me know when this is ready for QA.
(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.
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>
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>
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>
Nice work, Passing QA
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>
Pushed to master for 21.11, thanks to everybody involved!