Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop SQL upgrade scripts #291

Merged
merged 4 commits into from
Jun 8, 2022
Merged

Conversation

RKrahl
Copy link
Member

@RKrahl RKrahl commented May 24, 2022

As discussed in the meeting on 19th May, we might want to drop the SQL upgrade scripts and rely on the initialization in icat.server provided by EclipseLink to upgrade the database instead.

This PR changes the eclipselink.ddl-generation setting from create-tables to create-or-extend-tables and drops the SQL upgrade scripts for version 5.0. The EclipseLink setting has the effect that the initialization code will not only add missing tables, but also add missing columns to existing tables.

Note that this works for simple additive schema changes as we had in recent versions and we envision for 5.0. More complex schema changes such as those discussed in #231 or #254 can not be covered automatically by the EclipseLink initialization and will still require dedicated upgrade scripts.

I tested this with MariaDB for the upgrade from 4.11.1 to 5.0 (the current icat-schema-extension branch). I even tested an upgrade from icat.server 4.7.0 to 5.0 in one single step without running any upgrade scripts. This works as well with one noteworthy exception: the EclipseLink initialization did not create the index on Datafile.location added in version 4.8. But our SQL upgrade scripts didn't care to do that either.

@kevinphippsstfc, could you please test this with Oracle?

@RKrahl RKrahl added this to the 5.0.0 milestone May 24, 2022
Copy link
Contributor

@kevinphippsstfc kevinphippsstfc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on Oracle and compared the database schemas (tables, indexes, constraints) and they were the same for a fresh 5.0.0-SNAPSHOT install as for a 4.11.1 schema that had been upgraded by the installation of the 5.0.0-SNAPSHOT.

I also ran the integration tests against the Oracle database.

Copy link
Contributor

@kevinphippsstfc kevinphippsstfc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realised that the installation HTML file also needs updating to reflect the fact that there are no longer upgrade files but that the schema upgrade for this version is automatic upon installation.

@RKrahl
Copy link
Member Author

RKrahl commented May 30, 2022

I realised that the installation HTML file also needs updating to reflect the fact that there are no longer upgrade files but that the schema upgrade for this version is automatic upon installation.

I wonder how we should deal with the schema upgrade instructions for very old (older than 4.7.0) versions. These would still require a stepwise upgrade from version to version with manual interventions in each step as described in the install instructions in 4.7.0 (and still in the current release). Should we keep these instructions?

I'd assume that in the meanwhile nobody is using that old versions in production anyway. So I'd be inclined to take the opportunity to drop these old instructions and simply state that direct upgrade is supported for ICAT 4.7.0 and newer. For older versions, one should first upgrade to 4.7.0 following the instructions provided with that version and then upgrade to 5.0.0 in a second step. Upgrade from 4.7.0 or newer is covered by the automatic initialization provided by EclipseLink.

@RKrahl
Copy link
Member Author

RKrahl commented May 30, 2022

I now dropped all old upgrade scripts and updated the install instructions as outlined in the last comment. Please have a look.

Copy link
Contributor

@kevinphippsstfc kevinphippsstfc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is time to remove the upgrade instructions for the really old versions and I think the way you have done it makes sense.

@kevinphippsstfc kevinphippsstfc merged commit 5cc4798 into icat-schema-extension Jun 8, 2022
@RKrahl RKrahl deleted the drop-sql-upgrade branch June 8, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants