BIND 10 #963: Utility to upgrade SQLite schema

[BIND 10 Development]

#963: Utility to upgrade SQLite schema
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  vorner
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  sqlite schema upgrade              |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => vorner


Comment:

 > Is the goal to upgrade any kind of database, or only SQLite3? If the
 second, would it be better named like b10-sqlite3util?
 I would suggest leaving it as b10-dbutil.  At the moment we only have one
 database so the name "b10-dbutil" avoids unncessary complication.  If we
 have multiple types of database, hopefully the utility could decide what
 type of database it is accessing and invoke the appropriate code.  That
 way it remains a general utility.

 > While it might be harder to implement, I'd rather take the default
 database from configuration, if possible. Or, to avoid confusion, have the
 user always enter the filename of the database (otherwise the user might
 have configured a different database, forgot about it and this one says
 the default is OK, but the server does not work).
 Given the nearness of the end of the year delivery, I've altered the code
 to require the name of the database to be given.  I suggest that a new
 ticket be opened to take the name from the configuration.

 > Is there any reason to limit the upgrade utility to one database at a
 time? I believe we support (or, if not now, will support) multiple SQLite3
 database files to serve from, it would make sense to just list them all at
 once.
 No, we could do that.  But if the upgrade of one failed we could be left
 in a position where some have been upgraded and some not.  Would that be
 confusing?

 > Similarly to utilities like fsck, would it make sense to exit with known
 non-zero exit code if the check says the database is not up to date? This
 would make it easier to use in automated scripts.
 I didn't do it on the grounds that a non-zero return status implied an
 error (probably with the access to the database) and made it more awkward
 for the test code to check the result.   This could be done in the future
 though.

 > Related to this ‒ while it would be out of the scope of this ticket...
 The data source already reads the schema table on startup to get the
 version of the database.  Changes made in #324 should cause a message to
 be logged if there is a version mis-match.


 > Is there any actual reason to implement it in shell? I don't say it is
 wrong, but it is unusual and inconsistent with the rest, so there should
 be a reason for it.
 The original version of utility was written in shell script (see the
 prototype in the attachment to this ticket) as the utility was originally
 envisaged as no more than a simple wrapper around the execution of an
 sqlite3 script.  As it grew, it was rewritten in Python to handle error
 cases.   However, by then the tests were in shell script as well, and it
 did not seem a good use of time to change them.  (Besides, it's not clear
 how to compare sqlite3 schema in Python, whereas it's simple in shell
 script).

 I wouldn't object to the tests being rewritten in Python, but I think it
 is a low-priority task.

 > Is there any reason to use doxygen for the shell code? I don't think
 doxygen extracts it (it doesn't do now for sure, because the directory is
 not listed, but I don't think it even can) and it clutters the comments
 when read plain-text.
 Only so that the comments are in the same (consistent) format as comments
 in the C++ and Python code.

 > I don't like this construction with x. I don't know where the idiom
 comes from, but in case any of the variables contain spaces, this might
 even be wrong:
 Sorry, too much autotools script programming.  Changed.

 > Also, these tests don't check separate functions (well, they hardly can
 from shell), so they should be considered system-level tests, not unit
 tests. I believe they should be moved there, then.
 Agreed.  Once converted to Python the functions just kept growing.  Some
 unit tests need to be written.

 > Does the test file need to be generated from .in? I didn't see any
 replacement performed.
 The names of the temporary files and the location of the test data
 directory (at the top of the file) need substituting during the build
 procedure.

 > I believe some files are missing from EXTRA_DIST. The tests fail in
 distcheck (but pass in check)
 Missing files added.

 > Did we already decided to use doxygen for python?
 No, but I think the discipline imposed by Doxygen gives more consistent
 documentation.

 > And, why use the @ sign if all the rest of the code uses \?
 With a string, a backslash introduces an escape character which, in my
 syntax-colouring editor, appears in a different colour.  This was
 annoying.

 > Even classes can have the """string""" documentation comment. Therefore
 for completeness and consistency, should the Database class should be
 documented that way, not in a comment before the class.
 Changed.

 > If it is all doxygen, then we should not miss:
 > upgrades
 Renamed to UPGRADES - done
 > !DbutilException
 Done.

 > About the user-level documentation, I believe we need at least little
 bit of it. Otherwise, are the users expected to guess to use it? Unless
 they can really use it, I don't see a reason to ship it.
 Agreed.  My comment was concerning the need to get the task finished for
 the sprint.  I've added b10-dbutil.xml which produces a "man" page for the
 utility.

 > Also, what will happen after the major version upgrade now? Will the
 current code reject it? I think as part of merging this, we should upgrade
 the version the code expects.
 Agreed.  Although this can be merged now, the upgrade should not be
 performed until the changes in ticket #324 have been done.

 > When deciding the name for the backup, there's a race condition. Is that
 OK?
 I think it is acceptable for now.  The backup file's name is
 <dbfile>.backup.  If it exists, -1, -2 etc. is appended to the name.  The
 race condition should only occur if two instances of the upgrade utility
 are trying to upgrade the same database at exactly the same time.  I
 suggest adding this as a low-priority ticket.

 > I believe that better than checking for some errors (like missing a line
 in the version table) and omitting all the rest, the get_version function
 could be better implemented using try ‒ catch (simply try getting the
 version in the most straight-forward way, it will raise some exception if
 it fails, be it problem with opening the database or indexing beyond the
 end of array when there's no line in the table).
 It has been modified, but only insofar as a "SELECT COUNT(*)" is not
 needed.  Just getting the results one at a time from the "SELECT *" gives
 us the number of rows in the table.

 If there are any SQL-related exceptions, they are unrecoverable - they are
 caught at the outer level.

 > This error is not really very helpful I think:
 On examination, the exception could have been generated if an earlier
 version of the utility were run against a later version of the database.
 This situation has been explicitly tested for.

 The point where the exception is raised can now only be reached if the
 sequence of upgrades in the tool is incorrect.  The message has been
 changed.

 > Will this tool be used to replace compatcheck/sqlite3-difftbl-check.py ?
 Ultimately it will, as it has the --check functionality.  The tool copes
 with the proposed new format of the schema_version table.

-- 
Ticket URL: <http://bind10.isc.org/ticket/963#comment:11>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development