In order to maintain database schema consistency between installations running the same Koha version, we must not either conditionally apply or skip a schema alteration. See previous discussion https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22887#c15 Discuss how updatedatabase.pl and the GUI (and install.pl) should work when we want to stop a problematic database update and leave room for sysadmin to fix it.
I start from where we left off at Bug 22887 comment 28. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22887#c28 (In reply to Jonathan Druart from comment #28) > It is what we are doing for years. I can find only one occurrence that conditionally either alters the schema or not, and leaves the admin with only a warning. And that was 12 days ago by Bug 18177. > Dying in the updatedatabase.pl will require more work than just adding a die > statement. We need to handle the error, propagate it to the UI. The way it's > done so far will make the change not trivial. I tested updating via the GUI with a die() and it seems to display it well. Also we do already have a die() statement in a database update, although a very old one - but I'm not sure whether Koha handled it better then. I doubt it. I'll add an attachment for you to see how it looks.
Created attachment 102521 [details] How update in the GUI looks when a die() is given
(In reply to Lari Taskula from comment #1) > I start from where we left off at Bug 22887 comment 28. > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22887#c28 > > (In reply to Jonathan Druart from comment #28) > > It is what we are doing for years. > > I can find only one occurrence that conditionally either alters the schema > or not, and leaves the admin with only a warning. And that was 12 days ago > by Bug 18177. Yes, that is what I meant, we usually do not deal with errors. We drop the column (with data or not), we add the FK without checking if it will be created or not. What is needed here is to surround each of our entry with an eval and catch any errors raised by the DBMS. If there is an error, we stop the update process and raise the error to the interface/script, waiting for the situation to be unblocked. On the other hand I can imagine that one could want to get all the errors at once, and deal with them in one go. That's the behavior what I would like to get if I had to deal with upgrade process. In that case I imagine a switch would be needed.
Here are some occurrences where we drop a message and continue the update process. 7285 print "Upgrade to $DBversion done (Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber) failed! Transaction aborted because $@\n"; 7286 eval { $dbh->rollback }; 8057 print "Upgrade to $DBversion failed (Bug 7372: Move road types from the roadtype table to the ROADTYPE authorised values.\nTransaction aborted because $@\n)"; 8058 $dbh->rollback; 17027 warn "Upgrade to $DBversion failed: $@\n"; 11100 print "Upgrade to $DBversion done (Bug 10020: This database contains data related to 'ethnicity'. No change will be done on the DB structure but note that the Koha codebase does not use it)\n"; 12596 print "WARNING: You have corrupted data in your items table!! The table contains $cnt references to biblio records that do not exist.\nPlease correct your data IMMEDIATELY after this upgrade and ma nually add the foreign key constraint for biblionumber in the items table.\n";
(In reply to Jonathan Druart from comment #4) > Here are some occurrences where we drop a message and continue the update > process. > ... Ah you're right, I only went through warn statements.
(In reply to Jonathan Druart from comment #3) > On the other hand I can imagine that one could want to get all the errors at > once, and deal with them in one go. That's the behavior what I would like to > get if I had to deal with upgrade process. In that case I imagine a switch > would be needed. Do you mean executing all the db updates and gather errors from all of them? If an error occurs we should stop immediately in order to avoid the next steps potentially causing damage to the database. I like the idea of exception handling for db updates, but this seems like a big change to the installer considering it already sort of works the way it is as shown in the screenshot.
(In reply to Lari Taskula from comment #6) > I like the idea of exception handling for db updates, but this seems like a > big change to the installer considering it already sort of works the way it > is as shown in the screenshot. Of course, if we have the resources for it, I think it's a great idea!
How it works so far: from the IU, the updatedatabase.pl is called, the outputs redirected to tmp files, reopened and displayed. That's ugly and does not let us much flexibility. I would like to rewrite it and have 2 scripts, the command line script, then the UI script. Both would call a method for each update step. So we could retrieve the outputs, exceptions or whatever we want easily. As I said I would prefer to have the whole thing ran in one go. And you don't:) That only means we need a switch that would be easy to implement if things are correctly written. I am happy to collect any ideas you have to improve the update process. I will try and work on that during the next dev cycle.
It should definitely stop when an error is found. Each DBRev should be run inside a transaction as well. And it should report meaningful things :-D
I agree there should be a consistent way to stop the upgrade process, and specially one that will allow to resume from the place it broke, but I think we should try to stop the upgrade process as little as possible.. meaning that for example if we add a FK on a nullable column, and there are some values that don't exists in the foreign table, instead of killing the process, we can copy the offending rows to some table we create, set the column to null, add the restriction and raise a warning..
A question, can we have upgrade tests?.. I mean, a way to set some tables in a certain condition, execute the upgrade process and check the results all within a single transaction to rollback? Is it possible?
Created attachment 116032 [details] [review] Bug 25078: [POC] Lazy version This is the lazy, quick and dirty, version. We use a try catch block and execute the db entry in a transaction. When I error is found, we stop. The main problem is that we cannot do anything with the raw output in the template.
Created attachment 116033 [details] [review] Bug 25078: [POC] Put the db entries in a structure to handle them better If we have have a hashref (ideally in a module!) we could have do something more powerful and deal correctly with them. More work is needed: - The "export" of the $db_entries hashref from updatedatabase.pl is really dirty, we don't want to do that - We should not need to eval { require $updatedatabase_path } from .pm, but retrieve it from another pm The main problems with this approach is: 1. Someone will have to move all(?) the entries into the new structure/file 2. Habit will change, updatedatabase.pl will become a 10 lines script that will loop over the db entries and display the output In my opinion it's what we want. Don't be afraid by the diff size, most of the changes is a move of the subroutine from updatedatabase.pl to C4/Installer.pm
Here we go, please have a look at those 2 patches. It's still a POC and some stuffs are really dirty. Test it, provide feedback, talk about it, and I will keep working on it!
Screenshot UI: https://snipboard.io/g5JfjL.jpg Screenshot CLI: https://snipboard.io/bUKpai.jpg
Created attachment 116034 [details] [review] Bug 25078: [POC] Put the db entries in a structure to handle them better If we have have a hashref (ideally in a module!) we could have do something more powerful and deal correctly with them. More work is needed: - The "export" of the $db_entries hashref from updatedatabase.pl is really dirty, we don't want to do that - We should not need to eval { require $updatedatabase_path } from .pm, but retrieve it from another pm The main problems with this approach is: 1. Someone will have to move all(?) the entries into the new structure/file 2. Habit will change, updatedatabase.pl will become a 10 lines script that will loop over the db entries and display the output In my opinion it's what we want. Don't be afraid by the diff size, most of the changes is a move of the subroutine from updatedatabase.pl to C4/Installer.pm
Screenshot UI: https://snipboard.io/lpETMg.jpg Screenshot CLI: https://snipboard.io/cCoGju.jpg
I like the look of the patches here.. but they won't apply at the moment.
Created attachment 116208 [details] [review] Bug 25078: [POC] Lazy version This is the lazy, quick and dirty, version. We use a try catch block and execute the db entry in a transaction. When I error is found, we stop. The main problem is that we cannot do anything with the raw output in the template.
Created attachment 116209 [details] [review] Bug 25078: [POC] Put the db entries in a structure to handle them better If we have have a hashref (ideally in a module!) we could have do something more powerful and deal correctly with them. More work is needed: - The "export" of the $db_entries hashref from updatedatabase.pl is really dirty, we don't want to do that - We should not need to eval { require $updatedatabase_path } from .pm, but retrieve it from another pm The main problems with this approach is: 1. Someone will have to move all(?) the entries into the new structure/file 2. Habit will change, updatedatabase.pl will become a 10 lines script that will loop over the db entries and display the output In my opinion it's what we want. Don't be afraid by the diff size, most of the changes is a move of the subroutine from updatedatabase.pl to C4/Installer.pm
Pushed to a remove branch, I won't be able to maintain those patches up-to-date because of the changes to updatedatabase.pl and Koha.pm https://gitlab.com/joubu/Koha/-/tree/bug_25078
Created attachment 117084 [details] [review] Bug 25078: [POC] Move each DBrev to a file
Going further a bit more, now we have 1 file (perl module) perl DB rev. We could improve the implementation a bit more and have a base DBRev module which would implement ->execute, ->output version_from_module should be remove (we now have the ->version method). The version could be a package var actually. However the description cannot, we should make it flexible (for example adding more description/alert/warning depending on the exec process) I need a bit of discussion here, and that we agree on an implementation before submitting something ready to be tested and integrated.
I suggest to go even further by removing the tie between Koha version and database updates. It is not needed that each database update increments the Koha version (and it gives RM/RMaints additional work that could be avoided). I wrote something with that idea in mind: https://gitlab.com/jajm/Koha/-/commits/migrations - https://gitlab.com/jajm/Koha/-/commit/58bb8addbd4bb8deedd4643b8c465d8ac1857d8f Basically it's like atomicupdate, but the filename is prefixed by a timestamp to keep the updates ordered and the filename is saved in a table once successfully executed (so that updates are not executed twice). No need to replace XXX by the correct version, and it should be backport-friendly (but only once this get pushed into stable releases... can't win everywhere :/) This makes the 'Version' system preference useless, so a lot of the patch is for removing usage of it. I realize that it's out of the scope of the current bug, but since other patches tend to go that way (splitting DB updates), I think that having another alternative is good for discussion.
(In reply to Julian Maurice from comment #24) > I think that having another alternative is good for discussion. +1 I like the idea of moving towards database migrations. I think that I'd posted about this previously, but now I can't find my comments. A lot of modern software uses database migrations. Kong uses its own Lua-based system, Keycloak uses Liquibase for database migrations, and the other day I wrote a simple PHP-based migrations system for a legacy app.
Migrations is a really interesting idea.. I'm not sure why I'd not thought of it.. certainly worth considering.
I suggested that approach ~8 years ago, and it got abandoned (bug 7167). I am not against the idea but it's weird we finally get back to it... I personally don't think we should get rid of the "version" syspref. Mainly because of this: if ( Koha::Migrations->pending_migrations > 0 ) { We are going to execute it for every hit (at the OPAC at least): 1 SQL query + 1 list of the directory.
(In reply to Jonathan Druart from comment #27) > I suggested that approach ~8 years ago, and it got abandoned (bug 7167). I > am not against the idea but it's weird we finally get back to it... Maybe you were a misunderstood avant-gardiste :-) On a more serious note, I really liked the idea of bug 7167 at the time, but I think it tried to do too much at once (with a UI, and ability to re-execute failed DB updates from the UI, ...), which made it complicated to test > I personally don't think we should get rid of the "version" syspref. Mainly > because of this: > if ( Koha::Migrations->pending_migrations > 0 ) { > > We are going to execute it for every hit (at the OPAC at least): 1 SQL query > + 1 list of the directory. There are probably ways of making it less painful for performance, like caching the directory listing in memcached. The Version syspref is in cache too, so it should not change anything from a "behaviour" point of view. Another way could be to move the check to the start of the application (in the .psgi), so that it's checked only once (if we update the sources, we have to restart starman anyway) This implementation is a proof of concept. I'm sure it can be improved.
About the performance of directory listing, I ran a small test: % mktemp -d /tmp/tmp.t2Pdul9xNn % grep 'DBversion =' installer/data/mysql/updatedatabase.pl | wc -l 1240 # That is the number of DB updates since 3.0 % for i in $(seq 1 1240); do touch /tmp/tmp.t2Pdul9xNn/$(openssl rand -hex 32); done % perl -MTime::HiRes=tv_interval,gettimeofday -E 'my $t0 = [gettimeofday]; opendir(my $dh, $ARVG[0]); my @names; foreach my $fname (sort readdir($dh)) { push @names, $fname; } closedir($dh); my $elapsed = tv_interval($t0); say 1000*1000*$elapsed . "µs";' /tmp/tmp.t2Pdul9xNn/ 25µs I ran it several times and the result is always between 10µs and 50µs, so... I think there are better things to worry about.
(In reply to Julian Maurice from comment #29) > About the performance of directory listing, I ran a small test: > > % mktemp -d > /tmp/tmp.t2Pdul9xNn > > % grep 'DBversion =' installer/data/mysql/updatedatabase.pl | wc -l > 1240 # That is the number of DB updates since 3.0 > > % for i in $(seq 1 1240); do touch /tmp/tmp.t2Pdul9xNn/$(openssl rand -hex > 32); done > > % perl -MTime::HiRes=tv_interval,gettimeofday -E 'my $t0 = [gettimeofday]; > opendir(my $dh, $ARVG[0]); my @names; foreach my $fname (sort readdir($dh)) > { push @names, $fname; } closedir($dh); my $elapsed = tv_interval($t0); say > 1000*1000*$elapsed . "µs";' /tmp/tmp.t2Pdul9xNn/ > 25µs > > I ran it several times and the result is always between 10µs and 50µs, so... > I think there are better things to worry about. Oops. My bad... perl did not warn about the typo (ARVG vs ARGV). I should learn to use -w more So the results are more between 1ms and 4ms :) Maybe some caching would be useful after all
(In reply to Julian Maurice from comment #28) > Another way could be to move the check to the start of the application (in > the .psgi), so that it's checked only once (if we update the sources, we > have to restart starman anyway) I think that this would be the best approach. The applications I use that use db migrations only do the check at application startup time. Not per request.
IMO we are going to far and the 2 approaches are not in opposition. We could go further on a separate bug report if we really want to. Here we want to improve the way the DBrevs are executed from 2 different scripts/methods (CLI and UI).
I don't even know what's going on anymore. What's the current proposal?
(In reply to David Cook from comment #33) > I don't even know what's going on anymore. What's the current proposal? Hum? There are 3 POC patches attached to this bug report, see their messages. Then Julian's patches is a separated patch. I am suggesting now to: - move forward with my patches with Julian's idea in mind (adjustments are needed) - move Julian's patch to a separate but report (adjustments will be needed as well)
(In reply to Jonathan Druart from comment #34) > (In reply to David Cook from comment #33) > > I don't even know what's going on anymore. What's the current proposal? > > Hum? There are 3 POC patches attached to this bug report, see their messages. > > Then Julian's patches is a separated patch. > > I am suggesting now to: > - move forward with my patches with Julian's idea in mind (adjustments are > needed) > - move Julian's patch to a separate but report (adjustments will be needed > as well) I will attach my patch to a separate bug report. Let's focus on stopping the updatedatabase process.
(In reply to Julian Maurice from comment #35) > (In reply to Jonathan Druart from comment #34) > > (In reply to David Cook from comment #33) > > > I don't even know what's going on anymore. What's the current proposal? > > > > Hum? There are 3 POC patches attached to this bug report, see their messages. > > > > Then Julian's patches is a separated patch. > > > > I am suggesting now to: > > - move forward with my patches with Julian's idea in mind (adjustments are > > needed) > > - move Julian's patch to a separate but report (adjustments will be needed > > as well) > > I will attach my patch to a separate bug report. Let's focus on stopping the > updatedatabase process. To be fair I should add that stopping the upgrade process when a SQL error occurs is already the current behaviour (since bug 25026). With these patches we will have each DBrev executed in a transaction and better error handling.
(In reply to Jonathan Druart from comment #34) > Hum? There are 3 POC patches attached to this bug report, see their messages. > It's not clear if they're all separate standalone patches or if they're combined. The third one doesn't even have a description beyond its title. I'm guessing they're all separate. But it sounds like you folk have this under control so I'll let you folk handle it.
> I will attach my patch to a separate bug report. Let's focus on stopping the > updatedatabase process. For those interested, it is here: Bug 25078
(In reply to Julian Maurice from comment #38) > For those interested, it is here: Bug 25078 I meant bug 27880
Didn't we decide it should depend on this one?
(In reply to Jonathan Druart from comment #40) > Didn't we decide it should depend on this one? I don't remember this decision... Actually I don't know how I can base my patch on this bug since it has 3 different proposals. I see it more as a 4th alternative.
(In reply to Julian Maurice from comment #41) > (In reply to Jonathan Druart from comment #40) > > Didn't we decide it should depend on this one? > > I don't remember this decision... That's what I understood from our previous comments: > > I am suggesting now to: > > - move forward with my patches with Julian's idea in mind (adjustments are > > needed) > > - move Julian's patch to a separate but report (adjustments will be needed > > as well) > I will attach my patch to a separate bug report. Let's focus on stopping the > updatedatabase process.
The two main needs here were: 1. Stop the process if an error occurs 2. Handle it nicely for the end users. 1. has been done by bug 25026. However we are missing the transactions and a correct error handling. 2. Is done on this patch set, by allowing the info from each DBrev to be accessed from 2 different places. This is what we need for 21.05. My 3 POCs are complementary (each one adding to the previous one) and the commit messages explain what they are doing, and/or the diff is trivial. What you are suggestion on bug 27880 is much bigger and will certainly have side-effects (drop of the version syspref and modification of the version format). That's definitely out of the scope and does not answer 2. I would be inclined to go with your design and help to get it in, if we all agree it's where we want to go. Unfortunately most of the time I am the one who needs to provide the additional and clean-up work, and I won't have the time to deal with that for 21.05.
So, the goal of this bug report is to go from attachment 102521 [details] to https://snipboard.io/lpETMg.jpg ? I suggest to change the bug title then, because it was not clear to me. From my point of view the updatedatabase is already stopped gracefully (with an ugly error message, yes, but the error is handled and shown to the user). I agree that bug 27880 is out of scope here, but it can answer problem number 2 quite easily. Maybe we can find an intermediate step before going to bug 27880, but without having to rewrite everything twice. For instance, keep the file naming conventions of 27880, but with the version added. Something like: 20210305153137-201200013-create-table-foo.pl (I prefer .pl over .pm because there is no need to duplicate the name inside the file) The database version can still be extracted from the file name, like in your 3rd patch. And the upgrade path to 27880 should be straightforward Is that what you had in mind when you said "- move forward with my patches with Julian's idea in mind (adjustments are needed)" ?
(In reply to Julian Maurice from comment #44) > So, the goal of this bug report is to go from attachment 102521 [details] to > https://snipboard.io/lpETMg.jpg ? Yes, and the transaction. > I suggest to change the bug title then, because it was not clear to me. From > my point of view the updatedatabase is already stopped gracefully (with an > ugly error message, yes, but the error is handled and shown to the user). Yes, it is now (with bug 25026 pushed), not when the bug report was opened (neither when I wrote the first version). > I agree that bug 27880 is out of scope here, but it can answer problem > number 2 quite easily. > > Maybe we can find an intermediate step before going to bug 27880, but > without having to rewrite everything twice. > For instance, keep the file naming conventions of 27880, but with the > version added. Something like: > 20210305153137-201200013-create-table-foo.pl (I prefer .pl over .pm because > there is no need to duplicate the name inside the file) > The database version can still be extracted from the file name, like in your > 3rd patch. And the upgrade path to 27880 should be straightforward > > Is that what you had in mind when you said "- move forward with my patches > with Julian's idea in mind (adjustments are needed)" ? Exactly! :)
This is ready for testing again. I'd like it to be pushed at the beginning of this release cycle, to avoid unnecessary work with updatedatabase.pl Here we are going to: - Move each DBrev into a separate .pl file - Make the display/output nice for CLI and UI - Execute each DBrev in a transaction Improvement to this (on a separate bug report, see bug 27880) could be: - Use the database to store the dbrevs that have been executed already - Remove versioning and use YYYY-MM-DD-bug-42424.pl instead of the version => Easier for backporting => No longer need to be idempotent => Track of the execution (error/success) in the DB
Created attachment 121909 [details] [review] Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout 5f9333ffda0 (master on 2021-06-14) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose)
Created attachment 121910 [details] [review] Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force
Created attachment 121911 [details] [review] Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force
Comment on attachment 121909 [details] [review] Bug 25078: Put db revs into different files to handle them better Review of attachment 121909 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/db_revs/skeleton.pl @@ +3,5 @@ > +{ > + bug_number => "BUG_NUMBER", > + description => "A single line description", > + # description => ["Multi", "lines", "description"], > + # description => sub { return ["Your dynamic description"] }, I think there is something wrong about multi-lines/dynamic description. Why do we need that ? I think that we are confusing the update's "description" (what the update is supposed to do) with the update's "report" (what the update has done). A dynamic description also implies that it should be generated after the update is done, because the 'description' sub might use some data from the 'up' sub, but what if we want to display a list of pending updates before executing them ?
(In reply to Julian Maurice from comment #50) > Comment on attachment 121909 [details] [review] [review] > Bug 25078: Put db revs into different files to handle them better > > Review of attachment 121909 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: installer/data/mysql/db_revs/skeleton.pl > @@ +3,5 @@ > > +{ > > + bug_number => "BUG_NUMBER", > > + description => "A single line description", > > + # description => ["Multi", "lines", "description"], > > + # description => sub { return ["Your dynamic description"] }, > > I think there is something wrong about multi-lines/dynamic description. Why > do we need that ? I think that we are confusing the update's "description" > (what the update is supposed to do) with the update's "report" (what the > update has done). How that? I don't understand. We have several examples in the past where we have several lines. Basically a DBrev can contain several things. To prevent long line we split it and display 1 action per bullet point. > A dynamic description also implies that it should be generated after the > update is done, because the 'description' sub might use some data from the > 'up' sub, but what if we want to display a list of pending updates before > executing them ? It's not what is done (and has never been done). We display display the output when the DBrev has been executed. Are you suggesting you should change this behaviour?
(In reply to Jonathan Druart from comment #51) > Are you suggesting you should change this behaviour? s/you should/we should :D
TODO: Fix NewVersion()! But it does not prevent testing and feedback.
(In reply to Jonathan Druart from comment #51) > It's not what is done (and has never been done). We display display the > output when the DBrev has been executed. You're right, the output has always been displayed after the execution. What I'm saying is that it would be nice to be able to display a description of the updates before their execution. I know it's not the goal of this bug, but since it changes the structure of database updates, it's a good time to think about future improvements, to avoid changing the structure again later. I'll submit a patch later to show what I have in mind (maybe on a remote git repository to not pollute this bug)
(In reply to Julian Maurice from comment #54) > (In reply to Jonathan Druart from comment #51) > > It's not what is done (and has never been done). We display display the > > output when the DBrev has been executed. > You're right, the output has always been displayed after the execution. What > I'm saying is that it would be nice to be able to display a description of > the updates before their execution. > I know it's not the goal of this bug, but since it changes the structure of > database updates, it's a good time to think about future improvements, to > avoid changing the structure again later. > I'll submit a patch later to show what I have in mind (maybe on a remote git > repository to not pollute this bug) Then we would need 2 kinds of "description", as we want to keep the ability to output something dependent on what is in DB (like raise a warning if we find data inconsistency).
(In reply to Julian Maurice from comment #54) > I'll submit a patch later to show what I have in mind (maybe on a remote git > repository to not pollute this bug) https://gitlab.com/jajm/Koha/-/commit/864372e553d312d1a533a75f389d9a7aa143c62e (In reply to Jonathan Druart from comment #55) > Then we would need 2 kinds of "description", as we want to keep the ability > to output something dependent on what is in DB (like raise a warning if we > find data inconsistency). I agree (but I think that data inconsistency, or any other anomaly detected should stop the update until it's fixed)
(In reply to Julian Maurice from comment #56) > (In reply to Julian Maurice from comment #54) > > I'll submit a patch later to show what I have in mind (maybe on a remote git > > repository to not pollute this bug) > https://gitlab.com/jajm/Koha/-/commit/ > 864372e553d312d1a533a75f389d9a7aa143c62e Yes, good idea! Not sure why you absolutely want to pass $dbh however :) > (In reply to Jonathan Druart from comment #55) > > Then we would need 2 kinds of "description", as we want to keep the ability > > to output something dependent on what is in DB (like raise a warning if we > > find data inconsistency). > I agree (but I think that data inconsistency, or any other anomaly detected > should stop the update until it's fixed) We had example where we removed an "unused" DB column but wanted to prevent deletion if it contains data ("Hey, we were going to remove this column but you have data in it"). We could also stop the update in that case but it's not blocker. 20.06.00.055 is another example ("FK added" or "FK already existed"); Anyway your approach deals with that!
Note to myself, the first patch should: - my $report = update( $files, { force => 1 } ); + my $report = update( $files, { force => $force } );
(In reply to Jonathan Druart from comment #57) > (In reply to Julian Maurice from comment #56) > > https://gitlab.com/jajm/Koha/-/commit/ > > 864372e553d312d1a533a75f389d9a7aa143c62e > > Yes, good idea! > Not sure why you absolutely want to pass $dbh however :) Just anticipating a future where C4::Context->dbh disappear or is moved to Koha namespace. DB update files won't need to be modified if $dbh is passed as a parameter.
Created attachment 121982 [details] [review] Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout 5f9333ffda0 (master on 2021-06-14) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose)
Created attachment 121983 [details] [review] Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force
Created attachment 121984 [details] [review] Bug 25078: Re-introduce NewVersion This is ugly, we re-add the code we removed in the previous patch. We need to continue supporting "old" versions.
This has moved a bit.. looking good. I wonder about the --force option we've introduced a little.. might be good to have a 'force_once' or something so allow the user to pick to ignore the first error but have subsequent errors still catch. Generally, I think I'm happy here.. I do think Julians $report way of doing multi-line description blocks is cleaner looking somehow.. and I can see his point about passing dbh rather than suggesting we rely on C4::Context.. it is somewhat future-proofing (and it's a familiar concept to me to pass dbh around this way as it's how DBIx::Class::DeploymentHandler does it (though in that case.. they often also pass a dbic schema for the before and after upgrade step so you can use dbic during upgrades.. takes some getting used to, but is pretty cool) Also, bug 27880 looks really interesting too.. I hope we could quickly work through that after this.. feels like they lend themselves to being pushed in quick sucession.
(In reply to Martin Renvoize from comment #63) > This has moved a bit.. looking good. > > I wonder about the --force option we've introduced a little.. might be good > to have a 'force_once' or something so allow the user to pick to ignore the > first error but have subsequent errors still catch. I added the --force for testing purpose, we can remove it. > Generally, I think I'm happy here.. I do think Julians $report way of doing > multi-line description blocks is cleaner looking somehow.. and I can see his > point about passing dbh rather than suggesting we rely on C4::Context.. it > is somewhat future-proofing (and it's a familiar concept to me to pass dbh > around this way as it's how DBIx::Class::DeploymentHandler does it (though > in that case.. they often also pass a dbic schema for the before and after > upgrade step so you can use dbic during upgrades.. takes some getting used > to, but is pretty cool) Julian, could you attach a patch here?
Created attachment 122208 [details] [review] Bug 25078: Separate update "report" from its description
Comment on attachment 122208 [details] [review] Bug 25078: Separate update "report" from its description Review of attachment 122208 [details] [review]: ----------------------------------------------------------------- ::: C4/Installer.pm @@ +724,5 @@ > > my $error; > > + my $out = ''; > + open my $outfh, '>', \$out; About that feature of 'open', perldoc says: This feature works only when Perl is built with PerlIO -- the default, except with older (pre-5.16) Perl installations that were configured to not include it (e.g. via Configure -Uuseperlio). You can see whether your Perl was built with PerlIO by running perl -V:useperlio. If it says 'define', you have PerlIO; otherwise you don't. But it also says: For Perls 5.8.0 and later, PerlIO is (most often) the default It tested up to 5.10 (with docker images at https://hub.docker.com/_/perl) and it works well. If we want to be extra safe we can increase the minimum perl version required to 5.16 but I don't think it's necessary.
Created attachment 122322 [details] [review] Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout 5f9333ffda0 (master on 2021-06-14) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122323 [details] [review] Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122324 [details] [review] Bug 25078: Re-introduce NewVersion This is ugly, we re-add the code we removed in the previous patch. We need to continue supporting "old" versions. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122325 [details] [review] Bug 25078: Separate update "report" from its description Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122326 [details] [review] Bug 25078: (follow-up) Update 'NewVersion' for output_version changes This patch updates the NewVersion compatability method to add arrayref description handling to split it into description + report. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Looking good to me, signing off.
Created attachment 122329 [details] [review] Bug 25078: Another Test for the backward shim
Just added a follow-up atomicupdate (do not push it) to show how I tested that backwards compatibility stuff.
description => [qw(Multi lines description)], It's multiline and it doesn't matter that we actually use it, just having this syntax work is enough right? description => ["Testing", "multi lines", " failure"], up => sub { my $dbh = C4::Context->dbh; $dbh->do(q{ALTER TABLE Foo}); }, Is that useful have another test for this multi line case but successful? Should we have a failing test for this syntax? qw(Multi lines description) > - Make sure the different use cases are covered Seems good but quite over my head.
I get the following conflict when applying the rest of patches after «- Make sure the different use cases are covered» <<<<<<< HEAD ---- this is the code after applying «DB revs for testing» $VERSION = "21.06.00.100"; ||||||| constructed merge base $VERSION = "21.06.00.004"; --- code in master before the patch that I'm applying ======= $VERSION = "21.06.00.005"; --- code from the patch that I'm applying >>>>>>> Bug 25078: Put db revs into different files to handle them better I should keep 21.06.00.100 right? But I'm confused that «Put db revs into different [...]» changes the version.
Created attachment 122665 [details] [review] Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout c4b4db21d25 (master on 2021-07-08) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122666 [details] [review] Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122667 [details] [review] Bug 25078: Re-introduce NewVersion This is ugly, we re-add the code we removed in the previous patch. We need to continue supporting "old" versions. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122668 [details] [review] Bug 25078: Separate update "report" from its description Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122669 [details] [review] Bug 25078: (follow-up) Update 'NewVersion' for output_version changes This patch updates the NewVersion compatability method to add arrayref description handling to split it into description + report. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122670 [details] [review] Bug 25078: [DO NOT PUSH] Another Test for the backward shim
Created attachment 122671 [details] [review] Bug 25078: Move DB revs up to 21.06.00.005
(In reply to Victor Grousset/tuxayo from comment #75) > description => [qw(Multi lines description)], > > It's multiline and it doesn't matter that we actually use it, just having > this syntax work is enough right? > > > description => ["Testing", "multi lines", " failure"], > up => sub { > my $dbh = C4::Context->dbh; > $dbh->do(q{ALTER TABLE Foo}); > }, > > Is that useful have another test for this multi line case but successful? > > Should we have a failing test for this syntax? > qw(Multi lines description) > > > - Make sure the different use cases are covered > > Seems good but quite over my head. I don't understand what you mean exactly. The description can return a scalar (one line description) or an arrayref (multi lines description). [qw(Multi lines description)] is similar to ["Multi", "lines", " description"], if it it what disturb you. (In reply to Victor Grousset/tuxayo from comment #76) > I get the following conflict when applying the rest of patches after «- Make > sure the different use cases are covered» > > <<<<<<< HEAD ---- this is the code after applying «DB revs for testing» > $VERSION = "21.06.00.100"; > ||||||| constructed merge base > $VERSION = "21.06.00.004"; --- code in master before the patch that I'm > applying > ======= > $VERSION = "21.06.00.005"; --- code from the patch that I'm applying > >>>>>>> Bug 25078: Put db revs into different files to handle them better > > I should keep 21.06.00.100 right? > But I'm confused that «Put db revs into different [...]» changes the version. I rebased the patch, adjusted the commit messages and added a new patch to move the 5 last db revs.
Note that there is an encoding issue (on 210600000.pl), I didn't manage to fix so far. I will ask Julian or have a look at it later.
Created attachment 122679 [details] [review] Bug 25078: Close open filehandle
Created attachment 122680 [details] [review] Bug 25078: Fix encoding issue with db_revs output From `perldoc -f open`: The scalars for in-memory files are treated as octet strings: unless the file is being opened with truncation the scalar may not contain any code points over 0xFF. So $out need to be decoded first in order to be used in other Perl strings
I tested this patch with the CLI and the Web installer. Both display the Unicode characters correctly.
(In reply to Jonathan Druart from comment #84) > [qw(Multi lines description)] is similar to ["Multi", "lines", " > description"], if it it what disturb you. That was it, thanks for the explanation.
Testing note: always restart services after changing the version syspref. ==== A bit confused about the test plan: > - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) > - Read the different DBrevs created as examples > - Make sure the different use cases are covered > - execute the updatedatabase script (CLI) Actually all patches should be applied from the start, right? Moving on with this assumption. ====== > - execute the updatedatabase script (CLI) output: > Wide character in say at /kohadevbox/koha/installer/data/mysql/db_revs/210600000.pl line 11. > Upgrade to 21.06.00.000 done [01:12:42]: Increase DBRev for 21.06 > - [U+1F3B5] Run, rabbit run. [U+1F3B6] > - Dig that hole, forget the sun, > - And when at last the work is done > - Don't sit down it's time to dig another one. > Upgrade to 21.06.00.001 done [01:12:42]: Bug 28489 - Modify sessions.a_session from longtext to longblob > Upgrade to 21.06.00.002 done [01:12:42]: Bug 28490 - Bring back accidentally deleted relationship columns > Upgrade to 21.06.00.003 done [01:12:43]: Bug 24434 - Add 'WrongTransfer' to branchtransfers.cancellation_reason enum > Upgrade to 21.06.00.004 done [01:12:43]: Bug 15788 - Split edit_borrowers permission > Upgrade to 21.06.00.005 done [01:12:43]: Bug 26205 - Add new system preference NewsLog to log news changes > Upgrade to 21.06.00.081 done [01:12:43]: Bug 42322 - Single line description, no report > Upgrade to 21.06.00.082 done [01:12:43]: Bug 42422 - Testing number of patrons > - Number of patrons: 53 > Upgrade to 21.06.00.083 done [01:12:43]: Bug 42323 - Multi lines report > - Multi > - lines > - report > Upgrade to 21.06.00.090 failed [01:12:43]: Bug 42420 - Testing failure > ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Table 'koha_kohadev.Foo' doesn't exist [for Statement "ALTER TABLE Foo"] at /kohadevbox/koha/C4/Installer.pm line 734 > > DEV atomic update: bug_25078.perl > Upgrade to XXX [01:12:43]: Bug 25078 - Description > - Boom, boom > - Chick-a-boom The Chick-a-boom atomic update is executed after the failure of the previous update. Is that an issue? «Wide character in say» Shouldn't it be fixed by «Fix encoding issue with db_revs output» ======= > - Update the DB from the UI ↓↓↓ > User koha_kohadev doesn't have enough privilege on database koha_kohadev. What?! This is obviously unrelated but worth mentioning in case others using koha-testing-docker or not also have that. Anyway, moving on. Ok lol, we can actually move on. ¯\_ (ツ)_/¯ ===== > DEV atomic update: bug_25078.perl > Upgrade to XXX [23:11:53]: Bug 25078 - Description > - Boom, boom > - Chick-a-boom > > Upgrade to 21.06.00.000 done [23:11:53]: Increase DBRev for 21.06 > - [U+1F3B5] Run, rabbit run. [U+1F3B6] > - Dig that hole, forget the sun, > - And when at last the work is done > - Don't sit down it's time to dig another one. > Upgrade to 21.06.00.001 done [23:11:53]: Bug 28489 - Modify sessions.a_session from longtext to longblob > Upgrade to 21.06.00.002 done [23:11:53]: Bug 28490 - Bring back accidentally deleted relationship columns > Upgrade to 21.06.00.003 done [23:11:53]: Bug 24434 - Add 'WrongTransfer' to branchtransfers.cancellation_reason enum > Upgrade to 21.06.00.004 done [23:11:53]: Bug 15788 - Split edit_borrowers permission > Upgrade to 21.06.00.005 done [23:11:53]: Bug 26205 - Add new system preference NewsLog to log news changes > Upgrade to 21.06.00.081 done [23:11:53]: Bug 42322 - Single line description, no report > Upgrade to 21.06.00.082 done [23:11:53]: Bug 42422 - Testing number of patrons > - Number of patrons: 53 > Upgrade to 21.06.00.083 done [23:11:53]: Bug 42323 - Multi lines report > - Multi > - lines > - report > > Update error : > > Upgrade to 21.06.00.090 failed [23:11:53]: Bug 42420 - Testing failure > ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Table 'koha_kohadev.Foo' doesn't exist [for Statement "ALTER TABLE Foo"] at /kohadevbox/koha/C4/Installer.pm line 734 This time the Chick-a-boom update is first! Does it matter? And no «Wide character in say» warning. ====== Oh a "try again" button, let's press it. Same error, as expected. The Chick-a-boom atomic update is again ran. But we don't care, right? ====== --force test looks good :D Additional test: commenting the failing part of the transaction test caused the cities table to have a new line. As expected!
(In reply to Victor Grousset/tuxayo from comment #90) > Testing note: always restart services after changing the version syspref. > > ==== > > A bit confused about the test plan: > > > - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) > > - Read the different DBrevs created as examples > > - Make sure the different use cases are covered > > - execute the updatedatabase script (CLI) > > Actually all patches should be applied from the start, right? Yes, all the patches. > The Chick-a-boom atomic update is executed after the failure of the previous > update. > Is that an issue? No, after is correct. > «Wide character in say» Shouldn't it be fixed by «Fix encoding issue with > db_revs output» Should, yes. Looks like there is still an issue. Patch coming. > > - Update the DB from the UI > ↓↓↓ > > User koha_kohadev doesn't have enough privilege on database koha_kohadev. > > What?! This is obviously unrelated but worth mentioning in case others using > koha-testing-docker or not also have that. I have no idea what this is about. > This time the Chick-a-boom update is first! Does it matter? Not a big deal in the first step I'd say. Can be fix later if needed. The atomic updates are executed by updatedatabase.pl and should be moved to C4::Installer.
Created attachment 122875 [details] [review] Bug 25078: Fix 'Wide character' from CLI
As it, we are moving the atomic updates to the db_rev directory. Is it the behaviour we want? Should we 1. support both syntaxes (.perl and .pl) in installer/data/mysql/atomicupdate or 2. keep this directory for the old syntax and new atomic updates will be expected in installer/data/mysql/db_revs?
Created attachment 123356 [details] [review] Bug 25078: Keep atomic updates in "atomicupdate" dir The atomicupdate directory will contain all the atomic update files (old and new version). That will ease job for the RM and RMaints, no file (except skeletons) must be in this directory before push. This patch also fixes an inconsistency we had: the atomic update was run before the other db revs on the UI but after when the CLI script was used. Now we make sure that the CLI does not deal with the atomic update files when called from the installer (UI)
Created attachment 123357 [details] [review] Bug 25078: Keep atomic updates in "atomicupdate" dir The atomicupdate directory will contain all the atomic update files (old and new version). That will ease job for the RM and RMaints, no file (except skeletons) must be in this directory before push. This patch also fixes an inconsistency we had: the atomic update was run before the other db revs on the UI but after when the CLI script was used. Now we make sure that the CLI does not deal with the atomic update files when called from the installer (UI)
https://snipboard.io/W7skMG.jpg
Created attachment 123358 [details] [review] Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout c4b4db21d25 (master on 2021-07-08) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123359 [details] [review] Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123360 [details] [review] Bug 25078: Re-introduce NewVersion This is ugly, we re-add the code we removed in the previous patch. We need to continue supporting "old" versions. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123361 [details] [review] Bug 25078: Separate update "report" from its description Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123362 [details] [review] Bug 25078: (follow-up) Update 'NewVersion' for output_version changes This patch updates the NewVersion compatability method to add arrayref description handling to split it into description + report. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 123363 [details] [review] Bug 25078: Move DB revs up to 21.06.00.011 Updated on 2021-08-02 for 21.06.00.011
Created attachment 123364 [details] [review] Bug 25078: Close open filehandle
Created attachment 123365 [details] [review] Bug 25078: Fix encoding issue with db_revs output From `perldoc -f open`: The scalars for in-memory files are treated as octet strings: unless the file is being opened with truncation the scalar may not contain any code points over 0xFF. So $out need to be decoded first in order to be used in other Perl strings
Created attachment 123366 [details] [review] Bug 25078: Fix 'Wide character' from CLI
Created attachment 123367 [details] [review] Bug 25078: Keep atomic updates in "atomicupdate" dir The atomicupdate directory will contain all the atomic update files (old and new version). That will ease job for the RM and RMaints, no file (except skeletons) must be in this directory before push. This patch also fixes an inconsistency we had: the atomic update was run before the other db revs on the UI but after when the CLI script was used. Now we make sure that the CLI does not deal with the atomic update files when called from the installer (UI)
Patches (and remote branch) rebased on top of master.
(In reply to Jonathan Druart from comment #107) > Patches (and remote branch) rebased on top of master. Any patches I would attach here would be out of date. Jonathan, consider this bug passed qa. Can you just add my sign-off before pushing to master?
Awesome!
Created attachment 123850 [details] [review] Bug 25078: Move skeleton file
Created attachment 123851 [details] [review] Bug 25078: Don't process db revs if updatedatabase.pl failed The updatedatebase stops when something wrong happens, we should not continue to process the new DB revs (db_revs dir) in that case.
Pushed to master for 21.11, thanks to everybody involved!
Created attachment 123853 [details] [review] Bug 25078: Add exec flags
(In reply to Jonathan Druart from comment #113) > Created attachment 123853 [details] [review] [review] > Bug 25078: Add exec flags Pushed to master.
Created attachment 123937 [details] [review] Bug 25078: Explicitely return from DB revs to avoid warnings % perl -wc installer/data/mysql/atomicupdate/skeleton.pl Useless use of anonymous hash ({}) in void context at installer/data/mysql/atomicupdate/skeleton.pl
(In reply to Jonathan Druart from comment #115) > Created attachment 123937 [details] [review] [review] > Bug 25078: Explicitely return from DB revs to avoid warnings > > % perl -wc installer/data/mysql/atomicupdate/skeleton.pl > Useless use of anonymous hash ({}) in void context at > installer/data/mysql/atomicupdate/skeleton.pl Pushed to master.
Created attachment 124269 [details] [review] Bug 25078: Fix indentation and style for additional descriptions
(In reply to Jonathan Druart from comment #117) > Created attachment 124269 [details] [review] [review] > Bug 25078: Fix indentation and style for additional descriptions Pushed to master.
Note than new process is explained in wiki : https://wiki.koha-community.org/wiki/Database_updates#Pushing_to_a_branch Great job \o/