<p style="white-space: pre-wrap; word-wrap: break-word;">You're right in IRC, I have a lot of suggestions and also agree with Pau:</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">My humble idea would go as follows:</p><ul><li>hlr.sql contains the latest db schema (including setting the version)</li></ul><ul><li>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...).</li></ul><ul><li>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.</li></ul><ul><li>If the db was empty (no meta and no subscriber table), run db_bootstrap() creating the latest scheme from hlr.sql.</li></ul><ul><li>If the version already matched current, then be merry and nothing to do.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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)</p><p style="white-space: pre-wrap; word-wrap: break-word;">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...</p><p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/11898">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11898/4//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11898/4//COMMIT_MSG@14">Patch Set #4, Line 14:</a> <code style="font-family:monospace,monospace">Furthermore, introduce a new column to the subscriber table which is</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11898/4/sql/hlr.sql">File sql/hlr.sql:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11898/4/sql/hlr.sql@4">Patch Set #4, Line 4:</a> <code style="font-family:monospace,monospace">       value   TEXT NOT NULL</code></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">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<br>  <br>  version INTEGER</pre><p style="white-space: pre-wrap; word-wrap: break-word;">we can still always "select version from meta" backwards compatibly, and then read any other specific fields we might add in the future?</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">To criticise my own plan, the meta table would always need to be enforced to have exactly one row. What is making more sense?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11898">change 11898</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/11898"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-hlr </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8aeaa9a404b622657cbc7138106f38aa6ad8d01b </div>
<div style="display:none"> Gerrit-Change-Number: 11898 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 23 Nov 2018 15:31:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>