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.