Change in osmo-hlr[master]: add database schema versioning to the HLR database

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Fri Nov 23 15:31:03 UTC 2018


Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11898 )

Change subject: add database schema versioning to the HLR database
......................................................................


Patch Set 4: Code-Review-1

(2 comments)

You're right in IRC, I have a lot of suggestions and also agree with Pau:

I also kind of dislike that the current db schema is hidden in the upgrade code path. Instead, I would also prefer the current schema completely written out in hlr.sql, and figure out an upgrade path before. I understand that testing the upgrade path would be nice, and another drawback is that we would duplicate the commands to add new columns: once in the current hlr.sql and once in the upgrade path. But I think the cost of a reader having to head-scratch and figure to understand what the current db definition is is just too high. Using the current status should be canonical because everyone uses that all the time, and rather have an obscure upgrade path since that is rarely interesting to look at.

My humble idea would go as follows:

- hlr.sql contains the latest db schema (including setting the version)

- In an initial step determine the version. (If there's neither meta nor subscriber = new db, just call db_bootstrap(); only subscriber = v0 db; otherwise get meta.version...). 

- If needed, run upgrade commands. I think I would put them in a separate .sql file and make a string array of it, exactly like db_bootstrap.h is generated from hlr.sql; e.g. upgrade_0_to_1.sql generates upgrade_0_to_1.h? We'd probably rename db_bootstrap.sed to sql_to_c.sed and re-use. I mean, it's ok to do the upgrade with SQL strings directly in C, but so far we're keeping the SQL strings apart from C pretty nicely, and it's less cluttered to write SQL in a separate file without C quotes.

- If the db was empty (no meta and no subscriber table), run db_bootstrap() creating the latest scheme from hlr.sql.

- If the version already matched current, then be merry and nothing to do.

From before, we know whether a db_bootstrap() is even needed, only run that on an empty db. This way we could drop the weird "IF NOT EXISTS" from hlr.sql.

Also, I think an upgrade should be opt-in. Add a --db-allow-upgrade cmdline option and otherwise error out hinting at that option if an upgrade would be required.

To test the upgrade code path, we should probably add a tests/upgrade/ with a hlr_v0.sql that also adds some data, and write an explicit test (.sh?) to invoke 'osmo-hlr --db-allow-upgrade' and verify the result (`sqlite3 db .dump`?). (maybe useful for that would be a '--check-db-and-exit' option as well)

Sorry if this blows up the amount of work on this issue, but IMHO it is worth the effort to invent a good and proper db schema upgrade path, because we'll be using it for every other future upgrade. If you'd rather have someone else worry about that, then shout out. Personally I think it might be fun to pan this out...

https://gerrit.osmocom.org/#/c/11898/4//COMMIT_MSG
Commit Message:

https://gerrit.osmocom.org/#/c/11898/4//COMMIT_MSG@14
PS4, Line 14: Furthermore, introduce a new column to the subscriber table which is
when a commit log has a paragraph starting in the bottom with "Furthermore", that's an almost certain indicator that it should be a separate patch.

Let's add the schema and meta data first, and in a separate patch add the new subscriber column and just bump the db schema version.


https://gerrit.osmocom.org/#/c/11898/4/sql/hlr.sql
File sql/hlr.sql:

https://gerrit.osmocom.org/#/c/11898/4/sql/hlr.sql@4
PS4, Line 4: 	value	TEXT NOT NULL
Is this taken from some example, or why are you opening another layer of name-value on top of the name-value already supplied by SQL? wouldn't it be simpler to just have a single-row-table of
  
  version INTEGER

we can still always "select version from meta" backwards compatibly, and then read any other specific fields we might add in the future?

Otherwise we have to query each meta value separately, and cannot use sqlite's get-by-name API that already exists to get all values with a single row query.

To criticise my own plan, the meta table would always need to be enforced to have exactly one row. What is making more sense?



-- 
To view, visit https://gerrit.osmocom.org/11898
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aeaa9a404b622657cbc7138106f38aa6ad8d01b
Gerrit-Change-Number: 11898
Gerrit-PatchSet: 4
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Comment-Date: Fri, 23 Nov 2018 15:31:03 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181123/a44cf4ea/attachment.htm>


More information about the gerrit-log mailing list