BIND 10 #963: Utility to upgrade SQLite schema

[BIND 10 Development]

#963: Utility to upgrade SQLite schema
-------------------------------------+-------------------------------------
                   Reporter:  shane  |                 Owner:  UnAssigned
                       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 vorner):

 * owner:  vorner => UnAssigned


Comment:

 Hello

 Reassigning to unassigned, as requested by Stephen ‒ so this expects
 someone to pick it up for addressing the comments, not review.

 Replying to [comment:9 jreed]:
 > > 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:
 >
 > Using a character like "x" is very normal (see our autoconf generated
 configure script for many examples).  Some old shells and some /bin/[
 implementations don't work correctly if the quoted content is empty (maybe
 complain about expected argument missing). I don't think any of our build
 systems have this problem though.

 I know I see it often, but should we keep having the workaround for
 obviously severely broken and probably long fixed shell? It's ugly and
 people probably use it just out of habit. And even if we were to keep the
 `x`, the rest should be quoted.

 Anyway, I see Stephen already changed it.

 > > 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.
 >
 > Moved to where?

 To system tests/lettuce tests. It does a lot of copying files and starting
 new processes and such. And the style is similar to the ones in system
 tests as well. If it walks like a system test and quacks as a system test
 then let's consider it a duck.

 Replying to [comment:11 stephen]:
 > 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.

 Maybe a placing a comment like
 {{{
 # TODO: Once we support different database types, add auto-detection of
 type
 }}}
 or something, to make it obvious this one is expected to be extended in
 future, not a new tool written would be nice, then.

 > > 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.

 +1.

 > > 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?

 I don't think it would be confusing. If you `cat` multiple files and one
 of them gives an IO error, it bails out right there and complains. And as
 the tool can be safely run on a up-to-date database, fixing the problem
 and re-running with `/var/lib/b10/*.sqlite3` (or whatever the path is)
 would be OK.

 > > 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.

 Well, it is an error, because the database can't be used by the software.

 What I'm proposing is returning `2` for not up-to-date and `1` for
 anything else, so it can be easily recognized. And construction like:
 {{{
 if ! b10-dbutil "$DATABASE" ; then
         die "There's an issue with Your database and you should have a
 look."
 fi
 }}}
 is still useful in a rc startup script and it doesn't need to know if the
 problem is „The database is old“ or „Someone was having fun and dumped
 /dev/zero to the database file“.

 > > 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.

 Yes, that is true. But in this case b10-auth will refuse to start and will
 be „jumping“. The XfrIn/XfrOut will fail on an attempt to access the
 database and return `SERVFAIL`. But the overall system will start and
 present a false idea that it does something. What I had in mind was the
 „Fail as early as you can and as loudly as you can“.

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

 OK. Makes sense.

 > Agreed.  Once converted to Python the functions just kept growing.  Some
 unit tests need to be written.

 Would it make sense to provide a python unittest for some of the functions
 now, then? Or future cleanup task?

 > > 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.

 Hmm, that sounds like a good reason. But we might want to mention this on
 some call. I'll try not to forget.

 > > 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.

 Optimally together, not one after another. So we don't have master in
 inconsistent state either way.

 > > 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.

 Maybe replacing it within this ticket makes sense? When the code is
 available for it?

 About the logging, should it be another task as well?

 If nobody is opposed, I'd create the follow-up tasks (and there's quite
 few of them today O:-).

 Also, there are problems with the man page generation. Both `make` and
 `make distcheck` fail if run with default `configure` switches:
 {{{
 make[3]: Entering directory `/home/vorner/work/bind10/src/bin/dbutil'
 make[3]: Leaving directory `/home/vorner/work/bind10/src/bin/dbutil'
 make[3]: *** No rule to make target `b10-dbutil.8', needed by `distdir'.
 Stop.
 make[2]: *** [distdir] Error 1
 make[2]: Leaving directory `/home/vorner/work/bind10/src/bin'
 }}}

 {{{
 /bin/sed -e "s|@@PYTHONPATH@@|/home/vorner/testing/bind10/lib64/python3.2
 /site-packages|" \
        -e "s|@@SYSCONFDIR@@|/home/vorner/testing/bind10/etc|" \
        -e
 "s|@@LIBEXECDIR@@|/home/vorner/testing/bind10/libexec/bind10-devel|"
 dbutil.py >b10-dbutil
 make[5]: *** No rule to make target `b10-dbutil.8', needed by `all-am'.
 Stop.
 make[5]: *** Waiting for unfinished jobs....
 chmod a+x b10-dbutil
 }}}

 Configuring with `--enable-man` does not help:
 {{{
 xsltproc --novalid --xinclude --nonet -o b10-dbutil.8
 http://docbook.sourceforge.net/release/xsl/current/manpages/docbook.xsl
 ./bindctl.xml
 warning: failed to load external entity "./bindctl.xml"
 unable to parse ./bindctl.xml
 make[3]: *** [b10-dbutil.8] Error 6
 make[3]: Leaving directory `/home/vorner/work/bind10/src/bin/dbutil'
 }}}

 Also, this text of man page seems to have typo (printed to where?):
 {{{
 <para>Enable verbose mode.  Each SQL command issued by the
 utility will be printed to before it is executed.</para>
 }}}

 Thanks

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