<p style="white-space: pre-wrap; word-wrap: break-word;">the idea is to first add the schema version evaluation in one patch, without changing the schema, and making that user_version = 0. Then add the v1 column in a separate patch, bump the version and add the upgrade_to_v1. So the v1 patch really only is v1 related.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also open is the argument to add a --db-upgrade option, to avoid upgrading implicitly. Otherwise, only because the new intern tried the new version once the operator cannot operate anymore, and "sorry, there is no downgrade path".</p><p style="white-space: pre-wrap; word-wrap: break-word;">It should also be fairly easy to add a db-upgrade test. Thinking a shell script to pipe some sql to sqlite3, then start osmo-hlr --db-upgrade on it and see that an sqlite3 .dump produces expected results.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The above is what I'd like to see, unless it takes inifinitely too much time.</p><p>Patch set 5:<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>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11898/5/src/db.c">File src/db.c:</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/5/src/db.c@378">Patch Set #5, Line 378:</a> <code style="font-family:monospace,monospace"> rc = sqlite3_exec(dbc->db, "PRAGMA journal_mode=WAL; PRAGMA synchonous = NORMAL;", 0, 0, &err_msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(interesting typo in old code: "synchronous"<br>is that breaking something?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11898/5/src/db.c@383">Patch Set #5, Line 383:</a> <code style="font-family:monospace,monospace">  if (!db_is_bootstrapped(dbc)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">imagine some future scheme adds a new table. Then db_is_bootstrapped() will return false because one of the tables is missing? And then we try to populate a db assuming it is empty and fail even before we hit the db upgrade path we should be running instead?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe db_is_bootstrapped() should just check for the subscriber table and otherwise rely on the user_version.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Checking whether all tables are really there as expected might be an additional last step after all upgrades are done.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11898/5/src/db.c@403">Patch Set #5, Line 403:</a> <code style="font-family:monospace,monospace">                LOGP(DDB, LOGL_ERROR, "Unable to read user version number from database '%s'\n", dbc->fname);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is the user_version always present in an sqlite db? otherwise, if I read this right, when osmo-hlr now hits a current .db file, the version is -1 because there is none, and then we don't upgrade?</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: 5 </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-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 27 Nov 2018 22:21:12 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>