CheckSchema: Verify intermediate schema upgrades

[oxzi]

When skipping a version for an Icinga DB upgrade, all intermediate upgrade steps must be taken. While this is already stated in the documentation, it might be overlooked.

This happened for one community user, upgrading from v1.1.0 to v1.2.0, skipping the intermediate schema upgrade for v1.1.1.

https://community.icinga.com/t/icingadb-failing-exactly-5-minutes-after-start/13955

First, the necessity for all upgrades in their release order was made more prominent in the documentation, hoping that less users would ignore this when skimming the upgrade docs.

However, the real change here is adding another check to the CheckSchema function, verifying that all schema upgrades between the lowest known version and the highest known version in the icingadb_schema table exists. If an intermediate schema upgrade was skipped, as in the thread above, this raises a descriptive error.

[lippserd]

@Al2Klimov had an alternative idea: Is it possible to add a check/throw statement at the beginning of each schema upgrade file? That way, also manual migrations would be guarded.

[oxzi]

@Al2Klimov had an alternative idea: Is it possible to add a check/throw statement at the beginning of each schema upgrade file? That way, also manual migrations would be guarded.

While I really like this idea, I just had difficulties implementing this in a MySQL schema upgrade file in a compact, not-over-engineered way. First, MySQL's IF control structure cannot be used outside a procedure. I was unable to achieve an abortion with using the IF() function.

The shortest MySQL version I came up with looks like the following.

DROP PROCEDURE IF EXISTS schema_upgrade;

DELIMITER //
CREATE PROCEDURE schema_upgrade(expected_version SMALLINT, new_version SMALLINT)
    BEGIN
        SELECT version INTO @latest_version FROM icingadb_schema ORDER BY id DESC LIMIT 1;
        IF @latest_version != expected_version THEN
            SIGNAL SQLSTATE '45000'
                SET MESSAGE_TEXT = 'Unexpected latest schema version. Are all intermediate upgrades applied?';
        END IF;

        INSERT INTO icingadb_schema (version, timestamp) VALUES (new_version, UNIX_TIMESTAMP() * 1000);
    END //
DELIMITER ;

CALL schema_upgrade(6, 7);
DROP PROCEDURE schema_upgrade;

-- Actual schema upgrades is here.
SELECT 23;

While this still feels a bit bloated - and PostgreSQL is yet to come -, this needs to be copied to every new schema upgrade, which is a manual task we must not forget. Otherwise, the schema_upgrade procedure could be part of the regular schema, but there it would feel misplaced for me.

Don't get me wrong, I like the idea of having this logic in the database, but unless someone points me into a more practical direction, I am uncertain.

[lippserd]

Otherwise, the schema_upgrade procedure could be part of the regular schema, but there it would feel misplaced for me.

Don't get me wrong, I like the idea of having this logic in the database, but unless someone points me into a more practical direction, I am uncertain.

Including the procedure directly in the baseline schema for use in future upgrades makes sense to me. But I haven't thought about when you can actually rely on it in a schema upgrade, as it needs to be introduced with a schema upgrade first. I have some ideas that we can discuss tomorrow.

[Al2Klimov]

FWIW, division by zero didn't work as expected:

MariaDB [icingadb]> select 1/count(*) from icingadb_schema where version=42;
+------------+
| 1/count(*) |
+------------+
|       NULL |
+------------+
1 row in set, 1 warning (0.001 sec)

MariaDB [icingadb]>

The idea was, if it's missing, count(*) is zero, so 1/0. But I only got a warning, even with ERROR_FOR_DIVISION_BY_ZERO.

[oxzi]

@Al2Klimov, yes, I also had to realize that the MySQL CLI continues execution regardless of errors. That's another reason why I came up with the procedure, explicitly signaling the error. This thread also contains some insights: https://stackoverflow.com/questions/773889/way-to-abort-execution-of-mysql-scripts-raising-error-perhaps

[oxzi]

Just going to dump this here before I am forgetting this again: An Icinga DB user complained that the error message for a missed schema update is not so helpful, since it prints the schema versions instead of the Icinga DB versions, but even the schema updates are named after the Icinga DB version, not the internally used integer. A map translating this might be useful.

[oxzi]

I just came across another case in the community forum where a check like this would have helped. Here, the user has first skipped a version and then applied the missing schema twice. This is something we could have mitigated.

[oxzi]

This approach would tell a user that they have messed up and ended in an invalid state. However, unless they have a backup at hand, this does not really help.

Instead, we should tackle this issue by doing schema upgrades correctly, see #1061.

[julianbrost]

This approach would tell a user that they have messed up and ended in an invalid state.

Wouldn't being told that the database schema wasn't upgraded properly already be an improvement over failing with random database errors?

However, unless they have a backup at hand, this does not really help.

IMHO that sounds more dramatic than it really is. That would only be the case if there are nasty dependencies between the different upgrades. For the typical "add a column here" and "change a column type there", the order in which these are applied doesn't really matter, so often enough just applying the missing intermediate upgrade retroactively can fix the issue.

Was this PR really that far away from being in a useful state?

[oxzi]

Was this PR really that far away from being in a useful state?

Technically it was working since August 2024. However, the idea never caught momentum and I wanted to clean the issue tracker.

If you think this is useful and should be merged, please reopen and I would rebase it.

[julianbrost]

I'm in favor of doing that change, I just never looked into this in more detail as it never left the "changes requested" state nor anyone asked.

[lippserd]

I would undo this change. There is no code in this package that relates to this functionality. I think you refer to main.go and the auto-import CLI argument, but then this comment would always have to be aligned with changes in main.go.

[oxzi]

Yes, I was both referring to the usage in the main.go and hinting that this error might require an action. I removed the comment as you suggested.

[lippserd]

From my side, it looks good. Since you mentioned the confusion our users have regarding the incremental schema version numbers compared to the names of the upgrade scripts that have the Icinga DB version in which the script was introduced, I suggest introducing a map in our code so that we can list exactly which files need to be imported in the correct order if schema upgrades are necessary. But that would be the subject of a new PR if we want to do that.