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

[Neels Hofmeyr]

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 7: Code-Review-1

(5 comments)

I feel a bit apologetic to -1 again...

https://gerrit.osmocom.org/#/c/11898/7/sql/hlr_index.sql
File sql/hlr_index.sql:

https://gerrit.osmocom.org/#/c/11898/7/sql/hlr_index.sql@3
PS7, Line 3: CREATE UNIQUE INDEX idx_subscr_imsi ON subscriber (imsi);
we're now only using the hlr.sql file when the db is empty; what is the reason to keep this file separate? There is none, right, or am I missing something?


https://gerrit.osmocom.org/#/c/11898/7/src/db.c
File src/db.c:

https://gerrit.osmocom.org/#/c/11898/7/src/db.c@243
PS7, Line 243: static bool db_is_bootstrapped_v0(struct db_context *dbc)
this is still kind of a mix between "is_bootstrapped" and "is_correct".

What if a future db scheme would, for example, drop one of those tables? Since you're always calling this function in db_open() that would need revisiting here.

I think just checking for 'subscriber' presence to distinguish between v0-empty and v0-bootstrapped would have identical use with less tails attached.


https://gerrit.osmocom.org/#/c/11898/7/src/db.c@346
PS7, Line 346: 	if (!db_is_bootstrapped_v0(dbc)) {
let's break it down:

empty:

- user_version set to 0
- no table 'subscriber' present

v0:

- user_version is set to 0
- 'subscriber' present

vN:

- user_version is set to N

So, IIUC, checking table presence makes sense *only* if the user_version is set to 0, to distinguish between "empty" and "v0 bootstrapped"?

Hence I would expect first getting the user_version, then iff zero check db table presence (and it would suffice to check presence of only table 'subscriber', IMHO).

If then the verdict is "empty", go for bootstrapping?


https://gerrit.osmocom.org/#/c/11898/7/src/hlr.c
File src/hlr.c:

https://gerrit.osmocom.org/#/c/11898/7/src/hlr.c@651
PS7, Line 651: 	}
(For a unit test, it would be helpful to exit the program right after a db upgrade, so first up I thought, the --upgrade-db option should exit right away. Then again opening up the possibility to automatically upgrade the db when hlr service gets restarted would imply to not exit. So maybe the unit test use case needs a separate option like --check-db-and-exit. i.e. nm)


https://gerrit.osmocom.org/#/c/11898/7/src/hlr_db_tool.c
File src/hlr_db_tool.c:

https://gerrit.osmocom.org/#/c/11898/7/src/hlr_db_tool.c@64
PS7, Line 64: 	printf("  -U --upgrade-db            Allow HLR database schema upgrades.\n");
(the variables and other options are all called 'db-foo', this is the only one called 'foo-db')



-- 
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: 7
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-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Mon, 03 Dec 2018 13:47:50 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181203/6e64da37/attachment.htm>