Bug 25078 - Update DB process - wrap each DBRev inside a transaction and better error handling
Summary: Update DB process - wrap each DBRev inside a transaction and better error han...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL: https://gitlab.com/joubu/Koha/-/tree/...
Keywords: rel_21_11_candidate, RM_priority
Depends on: 25026
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-07 14:30 UTC by Lari Taskula
Modified: 2021-08-02 10:30 UTC (History)
10 users (show)

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


Attachments
How update in the GUI looks when a die() is given (499.06 KB, image/jpeg)
2020-04-07 14:36 UTC, Lari Taskula
Details
Bug 25078: [POC] Lazy version (1.07 KB, patch)
2021-01-29 14:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [POC] Put the db entries in a structure to handle them better (21.06 KB, patch)
2021-01-29 14:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [POC] Put the db entries in a structure to handle them better (21.06 KB, patch)
2021-01-29 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [POC] Lazy version (3.68 KB, patch)
2021-02-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [POC] Put the db entries in a structure to handle them better (21.06 KB, patch)
2021-02-02 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [POC] Move each DBrev to a file (15.52 KB, patch)
2021-02-19 13:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Put db revs into different files to handle them better (22.70 KB, patch)
2021-06-14 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] DB revs for testing (4.04 KB, patch)
2021-06-14 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] DB revs for testing (4.70 KB, patch)
2021-06-14 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Put db revs into different files to handle them better (22.70 KB, patch)
2021-06-15 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] DB revs for testing (4.70 KB, patch)
2021-06-15 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Re-introduce NewVersion (8.44 KB, patch)
2021-06-15 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Separate update "report" from its description (9.55 KB, patch)
2021-06-21 08:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25078: Put db revs into different files to handle them better (22.76 KB, patch)
2021-06-23 11:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] DB revs for testing (4.75 KB, patch)
2021-06-23 11:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25078: Re-introduce NewVersion (8.50 KB, patch)
2021-06-23 11:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25078: Separate update "report" from its description (9.70 KB, patch)
2021-06-23 11:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25078: (follow-up) Update 'NewVersion' for output_version changes (1.35 KB, patch)
2021-06-23 11:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25078: Another Test for the backward shim (1.11 KB, patch)
2021-06-23 12:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25078: Put db revs into different files to handle them better (22.26 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] DB revs for testing (4.77 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Re-introduce NewVersion (8.50 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Separate update "report" from its description (9.72 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: (follow-up) Update 'NewVersion' for output_version changes (1.35 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] Another Test for the backward shim (1.13 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Move DB revs up to 21.06.00.005 (9.08 KB, patch)
2021-07-08 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Close open filehandle (636 bytes, patch)
2021-07-08 14:04 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25078: Fix encoding issue with db_revs output (1.07 KB, patch)
2021-07-08 14:04 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25078: Fix 'Wide character' from CLI (1.27 KB, patch)
2021-07-16 08:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Keep atomic updates in "atomicupdate" dir (16.97 KB, patch)
2021-08-02 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Keep atomic updates in "atomicupdate" dir (16.25 KB, patch)
2021-08-02 10:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Put db revs into different files to handle them better (22.32 KB, patch)
2021-08-02 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: [DO NOT PUSH] DB revs for testing (4.77 KB, patch)
2021-08-02 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Re-introduce NewVersion (8.55 KB, patch)
2021-08-02 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Separate update "report" from its description (9.72 KB, patch)
2021-08-02 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: (follow-up) Update 'NewVersion' for output_version changes (1.35 KB, patch)
2021-08-02 10:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Move DB revs up to 21.06.00.011 (13.11 KB, patch)
2021-08-02 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Close open filehandle (638 bytes, patch)
2021-08-02 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Fix encoding issue with db_revs output (1.08 KB, patch)
2021-08-02 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Fix 'Wide character' from CLI (1.27 KB, patch)
2021-08-02 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25078: Keep atomic updates in "atomicupdate" dir (16.29 KB, patch)
2021-08-02 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2020-04-07 14:30:19 UTC
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.
Comment 1 Lari Taskula 2020-04-07 14:32:00 UTC
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.
Comment 2 Lari Taskula 2020-04-07 14:36:30 UTC
Created attachment 102521 [details]
How update in the GUI looks when a die() is given
Comment 3 Jonathan Druart 2020-04-07 15:22:55 UTC
(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.
Comment 4 Jonathan Druart 2020-04-07 15:32:12 UTC
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";
Comment 5 Lari Taskula 2020-04-08 13:26:22 UTC
(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.
Comment 6 Lari Taskula 2020-04-08 13:35:25 UTC
(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.
Comment 7 Lari Taskula 2020-04-08 13:37:09 UTC
(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!
Comment 8 Jonathan Druart 2020-04-08 14:01:58 UTC
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.
Comment 9 Tomás Cohen Arazi 2020-10-07 15:01:31 UTC
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
Comment 10 Agustín Moyano 2020-10-08 23:08:57 UTC
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..
Comment 11 Agustín Moyano 2020-10-09 18:25:41 UTC
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?
Comment 12 Jonathan Druart 2021-01-29 14:23:41 UTC
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.
Comment 13 Jonathan Druart 2021-01-29 14:23:44 UTC
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
Comment 14 Jonathan Druart 2021-01-29 14:25:31 UTC
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!
Comment 15 Jonathan Druart 2021-01-29 14:27:35 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2021-01-29 14:30:19 UTC
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
Comment 17 Jonathan Druart 2021-01-29 14:32:18 UTC
Screenshot UI:  https://snipboard.io/lpETMg.jpg
Screenshot CLI: https://snipboard.io/cCoGju.jpg
Comment 18 Martin Renvoize 2021-02-02 11:54:37 UTC
I like the look of the patches here.. but they won't apply at the moment.
Comment 19 Jonathan Druart 2021-02-02 12:05:51 UTC
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.
Comment 20 Jonathan Druart 2021-02-02 12:05:54 UTC
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
Comment 21 Jonathan Druart 2021-02-02 14:30:59 UTC
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
Comment 22 Jonathan Druart 2021-02-19 13:10:15 UTC
Created attachment 117084 [details] [review]
Bug 25078: [POC] Move each DBrev to a file
Comment 23 Jonathan Druart 2021-02-19 13:15:11 UTC
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.
Comment 24 Julian Maurice 2021-02-19 16:48:51 UTC
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.
Comment 25 David Cook 2021-02-22 03:05:02 UTC
(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.
Comment 26 Martin Renvoize 2021-02-22 09:28:41 UTC
Migrations is a really interesting idea.. I'm not sure why I'd not thought of it.. certainly worth considering.
Comment 27 Jonathan Druart 2021-02-22 13:36:32 UTC
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.
Comment 28 Julian Maurice 2021-02-22 14:42:05 UTC
(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.
Comment 29 Julian Maurice 2021-02-22 15:04:29 UTC
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.
Comment 30 Julian Maurice 2021-02-22 15:14:54 UTC
(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
Comment 31 David Cook 2021-02-23 01:26:55 UTC
(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.
Comment 32 Jonathan Druart 2021-02-23 08:40:14 UTC
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).
Comment 33 David Cook 2021-02-24 23:52:49 UTC
I don't even know what's going on anymore. What's the current proposal?
Comment 34 Jonathan Druart 2021-02-25 08:23:53 UTC
(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)
Comment 35 Julian Maurice 2021-02-25 08:44:49 UTC
(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.
Comment 36 Jonathan Druart 2021-02-25 09:02:05 UTC
(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.
Comment 37 David Cook 2021-02-26 00:07:14 UTC
(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.
Comment 38 Julian Maurice 2021-03-05 12:03:28 UTC
> 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
Comment 39 Julian Maurice 2021-03-05 12:04:07 UTC
(In reply to Julian Maurice from comment #38)
> For those interested, it is here: Bug 25078

I meant bug 27880
Comment 40 Jonathan Druart 2021-03-05 12:14:59 UTC
Didn't we decide it should depend on this one?
Comment 41 Julian Maurice 2021-03-05 12:51:23 UTC
(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.
Comment 42 Jonathan Druart 2021-03-05 13:08:42 UTC
(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.
Comment 43 Jonathan Druart 2021-03-05 13:09:51 UTC
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.
Comment 44 Julian Maurice 2021-03-05 14:42:58 UTC
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)" ?
Comment 45 Jonathan Druart 2021-03-05 15:09:42 UTC
(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! :)
Comment 46 Jonathan Druart 2021-06-14 13:26:43 UTC
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
Comment 47 Jonathan Druart 2021-06-14 13:29:50 UTC
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)
Comment 48 Jonathan Druart 2021-06-14 13:29:55 UTC
Created attachment 121910 [details] [review]
Bug 25078: [DO NOT PUSH] DB revs for testing

Use with the --force flag:

  % updatedatabase --force
Comment 49 Jonathan Druart 2021-06-14 13:39:13 UTC
Created attachment 121911 [details] [review]
Bug 25078: [DO NOT PUSH] DB revs for testing

Use with the --force flag:

  % updatedatabase --force
Comment 50 Julian Maurice 2021-06-14 14:37:55 UTC
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 ?
Comment 51 Jonathan Druart 2021-06-14 14:40:18 UTC
(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?
Comment 52 Jonathan Druart 2021-06-14 14:40:41 UTC
(In reply to Jonathan Druart from comment #51)
> Are you suggesting you should change this behaviour?

s/you should/we should :D
Comment 53 Jonathan Druart 2021-06-14 14:41:16 UTC Comment hidden (obsolete)
Comment 54 Julian Maurice 2021-06-14 15:03:28 UTC
(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)
Comment 55 Jonathan Druart 2021-06-14 15:12:26 UTC
(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).
Comment 56 Julian Maurice 2021-06-14 16:51:40 UTC
(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)
Comment 57 Jonathan Druart 2021-06-15 08:55:58 UTC
(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!
Comment 58 Jonathan Druart 2021-06-15 08:58:07 UTC Comment hidden (obsolete)
Comment 59 Julian Maurice 2021-06-15 09:17:08 UTC
(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.
Comment 60 Jonathan Druart 2021-06-15 09:40:59 UTC
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)
Comment 61 Jonathan Druart 2021-06-15 09:41:06 UTC
Created attachment 121983 [details] [review]
Bug 25078: [DO NOT PUSH] DB revs for testing

Use with the --force flag:

  % updatedatabase --force
Comment 62 Jonathan Druart 2021-06-15 09:41:14 UTC
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.
Comment 63 Martin Renvoize 2021-06-18 15:50:27 UTC
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.
Comment 64 Jonathan Druart 2021-06-21 07:34:27 UTC
(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?
Comment 65 Julian Maurice 2021-06-21 08:01:17 UTC
Created attachment 122208 [details] [review]
Bug 25078: Separate update "report" from its description
Comment 66 Julian Maurice 2021-06-21 08:06:19 UTC
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.
Comment 67 Martin Renvoize 2021-06-23 11:18:48 UTC
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>
Comment 68 Martin Renvoize 2021-06-23 11:18:53 UTC
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>
Comment 69 Martin Renvoize 2021-06-23 11:18:57 UTC
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>
Comment 70 Martin Renvoize 2021-06-23 11:19:02 UTC
Created attachment 122325 [details] [review]
Bug 25078: Separate update "report" from its description

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2021-06-23 11:19:10 UTC
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>
Comment 72 Martin Renvoize 2021-06-23 11:19:53 UTC
Looking good to me, signing off.
Comment 73 Martin Renvoize 2021-06-23 12:10:17 UTC
Created attachment 122329 [details] [review]
Bug 25078: Another Test for the backward shim
Comment 74 Martin Renvoize 2021-06-23 12:11:22 UTC
Just added a follow-up atomicupdate (do not push it) to show how I tested that backwards compatibility stuff.
Comment 75 Victor Grousset/tuxayo 2021-07-07 16:12:02 UTC
    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.
Comment 76 Victor Grousset/tuxayo 2021-07-07 16:16:15 UTC
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.
Comment 77 Jonathan Druart 2021-07-08 07:48:22 UTC
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>
Comment 78 Jonathan Druart 2021-07-08 07:48:28 UTC
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>
Comment 79 Jonathan Druart 2021-07-08 07:48:34 UTC
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>
Comment 80 Jonathan Druart 2021-07-08 07:48:40 UTC
Created attachment 122668 [details] [review]
Bug 25078: Separate update "report" from its description

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Jonathan Druart 2021-07-08 07:48:46 UTC
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>
Comment 82 Jonathan Druart 2021-07-08 07:48:53 UTC
Created attachment 122670 [details] [review]
Bug 25078: [DO NOT PUSH] Another Test for the backward shim
Comment 83 Jonathan Druart 2021-07-08 07:48:59 UTC
Created attachment 122671 [details] [review]
Bug 25078: Move DB revs up to 21.06.00.005
Comment 84 Jonathan Druart 2021-07-08 07:51:23 UTC
(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.
Comment 85 Jonathan Druart 2021-07-08 07:53:32 UTC
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.
Comment 86 Julian Maurice 2021-07-08 14:04:24 UTC
Created attachment 122679 [details] [review]
Bug 25078: Close open filehandle
Comment 87 Julian Maurice 2021-07-08 14:04:30 UTC
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
Comment 88 Julian Maurice 2021-07-08 14:06:12 UTC
I tested this patch with the CLI and the Web installer. Both display the Unicode characters correctly.
Comment 89 Victor Grousset/tuxayo 2021-07-11 03:36:20 UTC
(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.
Comment 90 Victor Grousset/tuxayo 2021-07-14 04:25:58 UTC
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
> 						   - 🎵 Run, rabbit run. 🎶
> 						   - 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
>         - 🎵 Run, rabbit run. 🎶
>         - 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!
Comment 91 Jonathan Druart 2021-07-16 08:00:41 UTC
(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.
Comment 92 Jonathan Druart 2021-07-16 08:02:53 UTC
Created attachment 122875 [details] [review]
Bug 25078: Fix 'Wide character' from CLI
Comment 93 Jonathan Druart 2021-08-02 09:09:52 UTC
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?
Comment 94 Jonathan Druart 2021-08-02 10:15:59 UTC
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)
Comment 95 Jonathan Druart 2021-08-02 10:17:48 UTC
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)
Comment 96 Jonathan Druart 2021-08-02 10:19:25 UTC
https://snipboard.io/W7skMG.jpg
Comment 97 Jonathan Druart 2021-08-02 10:29:31 UTC
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>
Comment 98 Jonathan Druart 2021-08-02 10:29:37 UTC
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>
Comment 99 Jonathan Druart 2021-08-02 10:29:43 UTC
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>
Comment 100 Jonathan Druart 2021-08-02 10:29:49 UTC
Created attachment 123361 [details] [review]
Bug 25078: Separate update "report" from its description

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 101 Jonathan Druart 2021-08-02 10:29:54 UTC
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>
Comment 102 Jonathan Druart 2021-08-02 10:30:00 UTC
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
Comment 103 Jonathan Druart 2021-08-02 10:30:05 UTC
Created attachment 123364 [details] [review]
Bug 25078: Close open filehandle
Comment 104 Jonathan Druart 2021-08-02 10:30:10 UTC
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
Comment 105 Jonathan Druart 2021-08-02 10:30:16 UTC
Created attachment 123366 [details] [review]
Bug 25078: Fix 'Wide character' from CLI
Comment 106 Jonathan Druart 2021-08-02 10:30:21 UTC
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)
Comment 107 Jonathan Druart 2021-08-02 10:30:45 UTC
Patches (and remote branch) rebased on top of master.