https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25078 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #5 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 127998 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127998&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #117835|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #6 from Julian Maurice <julian.maurice@biblibre.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |olli-antti.kivilahti@jns.fi --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- *** Bug 14698 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #9 from Julian Maurice <julian.maurice@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=34088 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #14 from Julian Maurice <julian.maurice@biblibre.com> --- (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)

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #20 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 153007 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153007&action=edit Bug 27880: Minor optimizations for Koha::Migrations::pending_migrations Can save a few milliseconds when there are a lot (> 1000) of migration files -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #21 from Julian Maurice <julian.maurice@biblibre.com> --- Note that comment 6 is still valid (there is still work to be done). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #23 from Julian Maurice <julian.maurice@biblibre.com> --- (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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #25 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #29 from Julian Maurice <julian.maurice@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #31 from Julian Maurice <julian.maurice@biblibre.com> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #33 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #35 from David Cook <dcook@prosentient.com.au> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 --- Comment #37 from David Cook <dcook@prosentient.com.au> --- (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... -- You are receiving this mail because: You are watching all bug changes.