Bug 27880 - Store each database migrations state in database
Summary: Store each database migrations state in database
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
: 7167 14698 (view as bug list)
Depends on: 25078
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-05 12:01 UTC by Julian Maurice
Modified: 2023-07-11 23:47 UTC (History)
6 users (show)

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


Attachments
Bug 27880: Database migrations in separate files (33.59 KB, patch)
2021-03-05 12:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27880: Store each database migrations state in database (30.46 KB, patch)
2021-11-25 10:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27880: Store each database migrations state in database (27.52 KB, patch)
2023-07-04 07:47 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 27880: Minor optimizations for Koha::Migrations::pending_migrations (1.25 KB, patch)
2023-07-04 07:47 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2021-03-05 12:01:51 UTC
The goals of this patch are:
- stop the growth of updatedatabase.pl file (which is nearly 24k lines long at the moment)
- reduce the manual work for RM/RMaints (copy/paste of code from atomicupdate to updatedatabase.pl + increase of Koha version)
- provide a simple framework for a better control of the database update process
Comment 1 Julian Maurice 2021-03-05 12:02:18 UTC
Created attachment 117835 [details] [review]
Bug 27880: Database migrations in separate files

The goals of this patch are:
- stop the growth of updatedatabase.pl file (which is nearly 24k lines
long at the moment)
- reduce the manual work for RM/RMaints (copy/paste of code from
atomicupdate to updatedatabase.pl + increase of Koha version)
- provide a simple framework for a better control of the database update
process

Database migrations are regular Perl scripts that return migration
metadata and code in a hashref.
Their filename is meaningful. It should always start with a timestamp
(in the form YYYYMMDDHHMMSS), which is used for executing migrations in
the correct order. It should also contain a short description.

Once a migration has been successfully executed, its name is stored into
a new table `executed_migrations`. This is how we avoid executing a
migration twice.

With this new system, the 'Version' system preference becomes useless,
as well as the 4th part of the Koha version. A good part of the patch is
about adapting the code to these changes.

Test plan:
1. Apply patch and restart starman
2. Go to the staff interface, you should be redirected to the installer
3. Enter your credentials, click on "Continue to the next step" until
   you arrive at the "Update database" step
4. You should now see the following message:
   Pending migrations:

     20201002135449-create-executed-migrations-table.pl
   Click on "Update your database"

5. Now you should see
   Update report :

   Migration done:
   20201002135449-create-executed-migrations-table.pl (Create table executed_migrations)

   Everything went okay. Update done.

6. Log into Koha
7. Verify that the About page shows the correct version
8. Verify that the 'Help' link in the top right corner redirects to the
   correct version of the manual

9. Now, do a fresh install and verify that the executed_migrations table
   exists and is not empty at the end of the install process
Comment 2 Jonathan Druart 2021-06-14 13:25:00 UTC
*** Bug 7167 has been marked as a duplicate of this bug. ***
Comment 3 Martin Renvoize 2021-11-25 10:00:19 UTC
So.. bits of this ended up getting merged into bug 25078.. I'm not sure what's still to do.. I think the recording of which updates have run which this bug does is a good idea still.. allowing us to make update idempotent by checking for previous runs of the update rather than trying to be clever with that actual statements themselves.

Thoughts Julian.. is this still something you're happy to pursue?
Comment 4 Julian Maurice 2021-11-25 10:23:08 UTC
Yes, this is something I still want to work on. I'll try to rebase the patch soon so it can be tested
Comment 5 Julian Maurice 2021-11-25 10:41:39 UTC
Created attachment 127998 [details] [review]
Bug 27880: Store each database migrations state in database

This patch is the continuation of bug 25078. It improves it by
decorrelating database migrations from Koha version number, which allows
to prevent that a database migration is executed twice, even if present
in two different stable releases.

Database migrations are regular Perl scripts that return migration
metadata and code in a hashref.
Their filename is meaningful. It should always start with a timestamp
(in the form YYYYMMDDHHMMSS), which is used for executing migrations in
the correct order. It should also contain a short description.

Once a migration has been successfully executed, its name is stored into
a new table `executed_migrations`. This is how we avoid executing a
migration twice.

With this new system, the 'Version' system preference becomes useless,
as well as the 4th part of the Koha version. A good part of the patch is
about adapting the code to these changes.

Test plan:
1. Apply patch and restart starman
2. Go to the staff interface, you should be redirected to the installer
3. Enter your credentials, click on "Continue to the next step" until
   you arrive at the "Update database" step
4. You should now see the following message:
   Pending migrations:

     20201002135449-create-executed-migrations-table.pl
   Click on "Update your database"

5. Now you should see
   Update report :

   Migration done:
   20201002135449-create-executed-migrations-table.pl (Create table executed_migrations)

   Everything went okay. Update done.

6. Log into Koha
7. Verify that the About page shows the correct version
8. Verify that the 'Help' link in the top right corner redirects to the
   correct version of the manual

9. Now, do a fresh install and verify that the executed_migrations table
   exists and is not empty at the end of the install process
Comment 6 Julian Maurice 2021-11-25 11:53:23 UTC
The patch is rebased and works, but there is still some work to do:
- figure what to do with the already existing db_revs/<version>.pl (I believe we can leave it like that, but maybe there's another, cleaner solution)
- pass the same parameters to the 'up' subroutine as with the db_revs system (for now only $dbh is passed, not the $out parameter)
Comment 7 Jonathan Druart 2021-12-15 09:46:04 UTC
*** Bug 14698 has been marked as a duplicate of this bug. ***
Comment 8 Katrin Fischer 2023-07-01 18:05:46 UTC
(In reply to Julian Maurice from comment #0)
> The goals of this patch are:
> - stop the growth of updatedatabase.pl file (which is nearly 24k lines long
> at the moment)
> - reduce the manual work for RM/RMaints (copy/paste of code from
> atomicupdate to updatedatabase.pl + increase of Koha version)
> - provide a simple framework for a better control of the database update
> process

It feels like the first 2 goals have been achieved with the move to atomicupdate files, leaving only the part for saving run updates to the database. I am not sure about the benefit of that as we already can tell what has been run from the version - but I might be persuaded?
Comment 9 Julian Maurice 2023-07-03 08:39:52 UTC
The goal is to reduce manual work.

RM/RMaint won't need to rename the file and update the version number. There will be no distinction between a "dev update" (located in atomicupdate directory) and a "prod update" (located in db_revs directory). It becomes just like any other piece of code: it doesn't need to be moved once pushed/backported.

Also DB updates will never be executed twice (currently this can happen when switching from a stable version to another, since DB updates are often backported) so they won't need to be idempotent and should be easier to write.
For instance there is no need to check if a column exists before adding it because that column cannot exist before the update was executed.
Comment 10 David Cook 2023-07-04 00:21:43 UTC
I'm a fan of storing database migrations state in the database. I use that strategy with other apps I write/support. 

But I'm not sure I follow the position below...

(In reply to Julian Maurice from comment #9)
> RM/RMaint won't need to rename the file and update the version number. There
> will be no distinction between a "dev update" (located in atomicupdate
> directory) and a "prod update" (located in db_revs directory). It becomes
> just like any other piece of code: it doesn't need to be moved once
> pushed/backported.

Depending on when the update is pushed/backported, wouldn't the RM/RMaint need to rename the file so that it fits into the right order? If the developer creates a migration in 2021 but it isn't pushed until 2023, it'll be out of order with all the other migrations. 

I suppose you could argue that it'll be applied at the right time because all the other migrations would've been applied already, but I don't think that would work for upgrades across multiple versions. 

But I might be misunderstanding what you're proposing here.

> Also DB updates will never be executed twice (currently this can happen when
> switching from a stable version to another, since DB updates are often
> backported) so they won't need to be idempotent and should be easier to
> write.
> For instance there is no need to check if a column exists before adding it
> because that column cannot exist before the update was executed.

I reckon it's always good practice to check before adding things. I've noticed a lot of database discrepancies, so you never know what Frankenstein's monster of a database you might have.
Comment 11 David Cook 2023-07-04 00:23:55 UTC
(In reply to Julian Maurice from comment #0)
> The goals of this patch are:
> - stop the growth of updatedatabase.pl file (which is nearly 24k lines long
> at the moment)
> - reduce the manual work for RM/RMaints (copy/paste of code from
> atomicupdate to updatedatabase.pl + increase of Koha version)
> - provide a simple framework for a better control of the database update
> process

Other work has already stopped the growth of updatedatabase.pl, fortunately.

I don't understand the manual work for RM/RMaints so maybe we can get a more in-depth explanation for that? 

I do want to reiterate that I think recording database migrations in the database is a good idea. Although what do we do for historical migrations? Do we just choose an arbitrary point to start recording from?
Comment 12 David Cook 2023-07-04 00:26:11 UTC
Julian, you might also find bug 34088 interesting.

In that case, I'm just short circuiting when the database version is equal to the code version, but if we stored migrations in the database I was wondering if there could be an easy way of telling whether or not there were any outstanding migrations to run (in a high performance way).
Comment 13 David Cook 2023-07-04 01:08:13 UTC
I think Koha::Migrations->pending_migrations is actually a significant win for these patches.

It gets a big hashref of all past migrations, and then it checks in O(1) time whether a file has already been used. 

So we only have 1 database hit, and we have an efficient check per file.

Of course, the time it takes to check for pending migrations will degrade over time as the list of migration files grows. But that's a problem that we already have.

So we'd still have a net win by moving to this mechanism.
Comment 14 Julian Maurice 2023-07-04 06:36:10 UTC
(In reply to David Cook from comment #10)
> Depending on when the update is pushed/backported, wouldn't the RM/RMaint
> need to rename the file so that it fits into the right order? If the
> developer creates a migration in 2021 but it isn't pushed until 2023, it'll
> be out of order with all the other migrations. 
I think it's not a problem. Migrations don't need to be run in "push order". What matters is that a migration is executed after any other migrations it might rely on, which are all the migrations already existing in master at the time of writing the new migration (+ any migrations contained in bug reports dependencies). The timestamp prefix ensures that.

> I suppose you could argue that it'll be applied at the right time because
> all the other migrations would've been applied already, but I don't think
> that would work for upgrades across multiple versions. 
I don't understand, or I don't agree :)
If stable has migrations A B C D E and oldstable has only A C E, when switching for oldstable to stable, migrations B and D will be executed.
Why do you think it wouldn't work ?

> I reckon it's always good practice to check before adding things. I've
> noticed a lot of database discrepancies, so you never know what
> Frankenstein's monster of a database you might have.
Yes I might have been over optimistic with this. Things can go wrong even when migrations are executed only once.

(In reply to David Cook from comment #11)
> I don't understand the manual work for RM/RMaints so maybe we can get a more
> in-depth explanation for that? 
Move/rename of the atomicupdate file + increase of Koha version. It won't be necessary with these patches.

> I do want to reiterate that I think recording database migrations in the
> database is a good idea. Although what do we do for historical migrations?
> Do we just choose an arbitrary point to start recording from?
Yes. The arbitrary point would be when this patch is pushed, which will record the first migration. It might be a good idea to push it early in a release cycle, before any migration.
Since it removes the 4th part of the version number, it may cause confusion if we do it after other migrations have been included
(23.11.06.000 -> 23.11.06.001 -> 23.11.06 (???))

(In reply to David Cook from comment #12)
> Julian, you might also find bug 34088 interesting.
> 
> In that case, I'm just short circuiting when the database version is equal
> to the code version, but if we stored migrations in the database I was
> wondering if there could be an easy way of telling whether or not there were
> any outstanding migrations to run (in a high performance way).
Hmm.. Probably not :/

(In reply to David Cook from comment #13)
> I think Koha::Migrations->pending_migrations is actually a significant win
> for these patches.
> 
> It gets a big hashref of all past migrations, and then it checks in O(1)
> time whether a file has already been used. 
> 
> So we only have 1 database hit, and we have an efficient check per file.
> 
> Of course, the time it takes to check for pending migrations will degrade
> over time as the list of migration files grows. But that's a problem that we
> already have.
It would be interesting to see how much time it takes to process a directory of 100, 1000 and 10000 entries (we are getting close to 1500 updates actually in master)
Comment 15 David Cook 2023-07-04 06:47:58 UTC
I don't have time to reply right now but hopefully I will tomorrow. 

(Lots of holidays at the moment so might be a bit out of touch.)
Comment 16 Katrin Fischer 2023-07-04 07:31:59 UTC
Hm, you pretend like sequence of execution does not matter, but indeed it does. A FK can only be added to a table that already exists, a column can only be added AFTER another column that is already there etc. The sequence and the way we sometimes need to shift data make it necessary that even if updates are idempotent or only run once, the sequence plays a role.
Comment 17 Julian Maurice 2023-07-04 07:37:19 UTC
(In reply to Julian Maurice from comment #14)
> It would be interesting to see how much time it takes to process a directory
> of 100, 1000 and 10000 entries (we are getting close to 1500 updates
> actually in master)

Just ran a quick test:

Time to execute pending_migrations with an empty executed_migrations table and:
- 100 migration files: <1ms
- 1000 migration files: ~1.2ms
- 10000 migration files: ~6ms

Time to execute pending_migrations with a full executed_migrations table and:
- 1000 migration files: ~1.6ms
- 10000 migration files: ~12ms

Note that I got these times with two small modifications I made to pending_migrations, which I will submit with the rebased patch, they save a few ms in the "10000 files" test.

One way to avoid slowdowns as the number of files grows is to check for pending migrations only once at the start of the application, and not at every request.
Comment 18 Julian Maurice 2023-07-04 07:44:44 UTC
(In reply to Katrin Fischer from comment #16)
> Hm, you pretend like sequence of execution does not matter, but indeed it
> does. A FK can only be added to a table that already exists, a column can
> only be added AFTER another column that is already there etc. The sequence
> and the way we sometimes need to shift data make it necessary that even if
> updates are idempotent or only run once, the sequence plays a role.

Sequence does matter indeed. But there are several different valid sequences of execution.
You can't write a migration to add a FK to a table before the migration to add the table has been written, so the timestamp for the FK migration will always be after the timestamp for the table migration. So these two migrations will always be executed in the correct order.
But if you have a migration that adds a column to table A and another migration that adds a column to table B, the order of execution of these two migrations does not matter.
Comment 19 Julian Maurice 2023-07-04 07:47:28 UTC
Created attachment 153006 [details] [review]
Bug 27880: Store each database migrations state in database

This patch is the continuation of bug 25078. It improves it by
decorrelating database migrations from Koha version number, which allows
to prevent that a database migration is executed twice, even if present
in two different stable releases.

Database migrations are regular Perl scripts that return migration
metadata and code in a hashref.
Their filename is meaningful. It should always start with a timestamp
(in the form YYYYMMDDHHMMSS), which is used for executing migrations in
the correct order. It should also contain a short description.

Once a migration has been successfully executed, its name is stored into
a new table `executed_migrations`. This is how we avoid executing a
migration twice.

With this new system, the 'Version' system preference becomes useless,
as well as the 4th part of the Koha version. A good part of the patch is
about adapting the code to these changes.

Test plan:
1. Apply patch and restart starman
2. Go to the staff interface, you should be redirected to the installer
3. Enter your credentials, click on "Continue to the next step" until
   you arrive at the "Update database" step
4. You should now see the following message:
   Pending migrations:

     20201002135449-create-executed-migrations-table.pl
   Click on "Update your database"

5. Now you should see
   Update report :

   Migration done:
   20201002135449-create-executed-migrations-table.pl (Create table executed_migrations)

   Everything went okay. Update done.

6. Log into Koha
7. Verify that the About page shows the correct version
8. Verify that the 'Help' link in the top right corner redirects to the
   correct version of the manual

9. Now, do a fresh install and verify that the executed_migrations table
   exists and is not empty at the end of the install process
Comment 20 Julian Maurice 2023-07-04 07:47:31 UTC
Created attachment 153007 [details] [review]
Bug 27880: Minor optimizations for Koha::Migrations::pending_migrations

Can save a few milliseconds when there are a lot (> 1000) of migration
files
Comment 21 Julian Maurice 2023-07-04 07:48:58 UTC
Note that comment 6 is still valid (there is still work to be done).
Comment 22 Katrin Fischer 2023-07-04 09:37:00 UTC
How and when is the timestamp set?
Comment 23 Julian Maurice 2023-07-04 09:42:28 UTC
(In reply to Katrin Fischer from comment #22)
> How and when is the timestamp set?

The timestamp is in the filename (e.g. 20230704114126-add-syspref.pl if you create the file on 2023-07-04 at 11:41:26)
Comment 24 Katrin Fischer 2023-07-04 19:50:46 UTC
(In reply to Julian Maurice from comment #23)
> (In reply to Katrin Fischer from comment #22)
> > How and when is the timestamp set?
> 
> The timestamp is in the filename (e.g. 20230704114126-add-syspref.pl if you
> create the file on 2023-07-04 at 11:41:26)

But I don't know when it will be pushed, to renaming will be necessary for the RM like they do now.
Comment 25 David Cook 2023-07-05 04:40:25 UTC
(In reply to Julian Maurice from comment #17)
> (In reply to Julian Maurice from comment #14)
> > It would be interesting to see how much time it takes to process a directory
> > of 100, 1000 and 10000 entries (we are getting close to 1500 updates
> > actually in master)
> 
> Just ran a quick test:
> 
> Time to execute pending_migrations with an empty executed_migrations table
> and:
> - 100 migration files: <1ms
> - 1000 migration files: ~1.2ms
> - 10000 migration files: ~6ms
> 
> Time to execute pending_migrations with a full executed_migrations table and:
> - 1000 migration files: ~1.6ms
> - 10000 migration files: ~12ms
> 
> Note that I got these times with two small modifications I made to
> pending_migrations, which I will submit with the rebased patch, they save a
> few ms in the "10000 files" test.
> 
> One way to avoid slowdowns as the number of files grows is to check for
> pending migrations only once at the start of the application, and not at
> every request.

I'm a bit surprised that pending_migrations ran that fast. It takes me a lot longer just to load C4::Context and get a database handle via C4::Context->dbh on koha-testing-docker. Did you run that on koha-testing-docker or on a server?

--

Regarding only checking for migrations at the start of the application, that's a good point. I've noticed that's what other (persistent) applications do as well. I think we have consensus at this point that Koha should always be run persistently, so I think that is a reasonable move to make.

That said, I'm less worried about slowdowns with the web UI vs updatedatabase.pl, since updatedatabase.pl is where I notice the worst performance impacts when installing a new koha-common package.

What do you think about bug 34088 in this context? If the code version and the database version match and there are no atomic updates to run, it exits out of updatedatabase.pl. Koha::Migrations does look pretty minimal, but I found C4::Context to be too slow, so I used Koha::Database::dbh() and Koha::Config for a database handle and config since they were much faster to load and use.
Comment 26 David Cook 2023-07-05 04:55:12 UTC
(In reply to Julian Maurice from comment #18)
> Sequence does matter indeed. But there are several different valid sequences
> of execution.
> You can't write a migration to add a FK to a table before the migration to
> add the table has been written, so the timestamp for the FK migration will
> always be after the timestamp for the table migration. So these two
> migrations will always be executed in the correct order.
> But if you have a migration that adds a column to table A and another
> migration that adds a column to table B, the order of execution of these two
> migrations does not matter.

I understand your logic here, but I just wonder if you're missing some scenarios.

I'm struggling to think of them, but there's just something bothering me in the back of my mind.

At the moment, I wonder about backporting causing problems...

Scenario:
Version 23.05 and Master
- Bug 1 : Add column A to Table A

Master:
- Bug 2 : Modify Column A in Table A
- Bug 3 : Drop Column A in Table A

23.05:
- Bug 3 : Drop Column A in Table A (backported)

--

When you upgrade from 23.05 to 23.11, the migration for Bug 2 will exist on the file system, but it won't be marked as executed in the database, so Koha will try to apply the migration for Bug 2, and it should generate an error.

(But to be fair, I think that this might actually be a problem with the current db_revs system as well... I'd have to test it...)
Comment 27 David Cook 2023-07-05 05:03:57 UTC
(In reply to David Cook from comment #26)
> (But to be fair, I think that this might actually be a problem with the
> current db_revs system as well... I'd have to test it...)

Actually, it might not be a problem with the current version, because db_revs also check their version against the database version.
Comment 28 Julian Maurice 2023-07-05 07:07:15 UTC
(In reply to David Cook from comment #25)
> I'm a bit surprised that pending_migrations ran that fast. It takes me a lot
> longer just to load C4::Context and get a database handle via
> C4::Context->dbh on koha-testing-docker. Did you run that on
> koha-testing-docker or on a server?

I opened the database connection before starting the timer. I wanted to measure only the database query + listing the files.
Here's the code: https://pastes.io/mf644kcqog

> What do you think about bug 34088 in this context? If the code version and
> the database version match and there are no atomic updates to run, it exits
> out of updatedatabase.pl. Koha::Migrations does look pretty minimal, but I
> found C4::Context to be too slow, so I used Koha::Database::dbh() and
> Koha::Config for a database handle and config since they were much faster to
> load and use.

C4::Context is useless in Koha::Migrations and can be replaced by Koha::Database. But it will still be slower than comparing two version numbers

(In reply to David Cook from comment #27)
> (In reply to David Cook from comment #26)
> > (But to be fair, I think that this might actually be a problem with the
> > current db_revs system as well... I'd have to test it...)
> 
> Actually, it might not be a problem with the current version, because
> db_revs also check their version against the database version.

With db_revs, in your scenario the order of update would be (23.05) Bug 1 -> Bug 3 -> (switch to 23.11) Bug 2 -> Bug 3
So I think we have the same problem. The only difference is that Bug 3 is executed twice.
Comment 29 Julian Maurice 2023-07-05 07:09:35 UTC
(In reply to Katrin Fischer from comment #24)
> But I don't know when it will be pushed, to renaming will be necessary for
> the RM like they do now.

The timestamp must be set to the time of writing, and it must not be changed at the time of push
Comment 30 Katrin Fischer 2023-07-08 13:35:10 UTC
(In reply to Julian Maurice from comment #29)
> (In reply to Katrin Fischer from comment #24)
> > But I don't know when it will be pushed, to renaming will be necessary for
> > the RM like they do now.
> 
> The timestamp must be set to the time of writing, and it must not be changed
> at the time of push

Sorry, but I don't believe that makes sense. If the timestamp determines sequence, we can't do that. We have patches waiting years, they would "slip between" previous patches where they might not fit - arguments about sequence of database changes have been made before.

I also think there is a point about comparing versions - we cannot slow down Koha that needs to constantly check if the database needs updating. If this makes the check slower, because much more complex, that would be an issue.
Comment 31 Julian Maurice 2023-07-10 09:31:36 UTC
(In reply to Katrin Fischer from comment #30)
> (In reply to Julian Maurice from comment #29)
> > The timestamp must be set to the time of writing, and it must not be changed
> > at the time of push
> 
> Sorry, but I don't believe that makes sense. If the timestamp determines
> sequence, we can't do that. We have patches waiting years, they would "slip
> between" previous patches where they might not fit - arguments about
> sequence of database changes have been made before.

If I understand correctly you are saying that database schema modifications must be applied in a strict order. But currently we have that only if we stay on master. When jumping from stable versions to stable versions, the order is not guaranteed.
Let's take an example:

If I'm on 22.05.10 and I jump to 22.11.06 and then to 23.05.01 the order of migrations will be (bug numbers):
- [jump to 22.11.06]
- [not 32330] (already applied because backported on 22.05),
- 30642,
- ...
- [jump to 23.05.01]
- [not 32330] (already applied because backported on 22.05)
- 27424
- 20256
- [not 30642] (backported on 22.11)
- ...

To summarize: 30642 -> ... -> 27424 -> 20256 -> ...

But if I'm on 22.05.10 and I jump to 23.05.01, the order will be

27424 -> 20256 -> 30642 -> ...

30642 can be applied before or after 27424 depending on how often Koha is updated.

Another example: bugs 33004 and 32799 are both in 22.11 and 23.05 but they were pushed in a different order. If on 22.11 the order will be 33004 -> ... -> 32799, but if going directly to 23.05 it will be 32799 -> 33004

I understand that doing it the way I suggest "feels wrong". It's not intuitive. But I encourage you to search and find an example that may cause problems with this system (and that doesn't cause problems with the current one).
If someone can find one I'd be happy to close this bug.

> I also think there is a point about comparing versions - we cannot slow down
> Koha that needs to constantly check if the database needs updating. If this
> makes the check slower, because much more complex, that would be an issue.

It really is a minor slowdown. 1.5ms for 1000 migration files (which represents two-thirds of the whole Koha history).
But I agree it's an unnecessary slowdown, and the check can moved to the start of the application, as suggested in comment 17. Because it's always faster to do nothing :)
Comment 32 David Cook 2023-07-11 00:21:43 UTC
Generally speaking, I think that I'm in favour of this work, but I suppose we're all risk averse.

If for some reason it does end up causing problems and we have to revert, would RM/RMaints just end up converting migrations into db_revs? If they're happy to do that, then it sounds like there might not actually be too much risk? 

Although one might argue that we might not notice the problem until the migrations are too well entrenched...
Comment 33 David Cook 2023-07-11 00:47:39 UTC
When in doubt, I like to look at existing implementations and resources.

Ruby on Rails is known for its migrations: https://guides.rubyonrails.org/v3.2/migrations.html#what-s-in-a-name

"For example Alice adds migrations 20080906120000 and 20080906123000 and Bob adds 20080906124500 and runs it. Alice finishes her changes and checks in her migrations and Bob pulls down the latest changes. When Bob runs rake db:migrate, Rails knows that it has not run Alice’s two migrations so it executes the up method for each migration.

Of course this is no substitution for communication within the team. For example, if Alice’s migration removed a table that Bob’s migration assumed to exist, then trouble would certainly strike."

This is similar to what I was saying at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880#c26

I don't know enough about the RM/RMaint process, but it sounds like they'll need to be very careful with backporting any migrations so that they don't allow conflicting migrations / migrations will need to be written carefully. 

But I don't think that's a reason not to proceed.
Comment 34 David Cook 2023-07-11 01:01:28 UTC
Looking at backported db_revs and I think it is like Julian says. 

The following two files are equivalent:
./installer/data/mysql/db_revs/221102001.pl
./installer/data/mysql/db_revs/221200004.pl

When you upgrade from 22.11.03 to 23.05.03, you will run ./installer/data/mysql/db_revs/221200004.pl even though ./installer/data/mysql/db_revs/221102001.pl has already run.

The database migration system actually should be better because you're less likely to run the same migration script twice. 

The migration for "./installer/data/mysql/db_revs/221102001.pl" would be pushed to master then backported to 22.11.03. You run it as part of 22.11.03, which means you don't run it again when you're upgrading to 23.05.03. Nice.

--

The only issue would be if there's a migration in 23.05.03 which conflicts with the database coming from 22.11.03, but that would already be a problem with updatedatabase.pl and db_revs anyways.

--

So overall I think it should be OK in that respect.
Comment 35 David Cook 2023-07-11 01:05:15 UTC
(In reply to Julian Maurice from comment #31)
> It really is a minor slowdown. 1.5ms for 1000 migration files (which
> represents two-thirds of the whole Koha history).
> But I agree it's an unnecessary slowdown, and the check can moved to the
> start of the application, as suggested in comment 17. Because it's always
> faster to do nothing :)

If we move it to the start of the application, I think we'd have to process upgrades automatically behind the scenes and not via the UI. 

That would leave the UI just for the initial install I think?

Honestly, for most people using Koha packages, that should be the workflow they experience anyway, so I think that should work out well.

Would you do this in plack.psgi or somewhere else?
Comment 36 Julian Maurice 2023-07-11 07:22:16 UTC
(In reply to David Cook from comment #35)
> If we move it to the start of the application, I think we'd have to process
> upgrades automatically behind the scenes and not via the UI.

It's an interesting option. But maybe Koha should warn before making changes to the database schema. Or administrators should be able to disable this feature if they want greater control.
Another option is to simply refuse to start and tell the administrator it should run updatedatabase.pl
Another one is to start a minimal app that includes only the web installer (but that would probably require another restart after the web installer is done).

> Would you do this in plack.psgi or somewhere else?

plack.psgi, and Koha::App::{Intranet,Opac}::startup for mojolicious apps
Comment 37 David Cook 2023-07-11 23:43:55 UTC
(In reply to Julian Maurice from comment #36)
> (In reply to David Cook from comment #35)
> > If we move it to the start of the application, I think we'd have to process
> > upgrades automatically behind the scenes and not via the UI.
> 
> It's an interesting option. But maybe Koha should warn before making changes
> to the database schema. Or administrators should be able to disable this
> feature if they want greater control.
> Another option is to simply refuse to start and tell the administrator it
> should run updatedatabase.pl
> Another one is to start a minimal app that includes only the web installer
> (but that would probably require another restart after the web installer is
> done).

I suppose another option would be to do the install/upgrade check at startup time and set a variable in a global/singleton like Koha::Installer, which would be checked by each worker... 

If "need_upgrade" is set, then they repeat the check, because only 1 worker will likely complete the install/upgrade process, and the other workers would be working with stale data...
Comment 38 David Cook 2023-07-11 23:47:21 UTC
I wonder sometimes how many people use the Debian packages versus other installation methods...

The Debian packages automatically run the database upgrade so the upgrade check at startup time should always be false in a test/prod setting.

For developers, expecting them to manually run "koha-upgrade-schema" seems fair to me.

So I imagine we're considering people who do source/tarball installs, or git/dev installs?

In theory, when they're doing installs/upgrades, they're using "make", so maybe we should add a database upgrade step there OR just add a warning that they should manually run it.