BIND 10 #324: improve SQL data source performance

[BIND 10 Development]

#324: improve SQL data source performance
-------------------------------------+-------------------------------------
                   Reporter:  each   |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
  performance                        |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
  sqlite schema upgrade              |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:22 jelte]:

 Thanks for the review.

 > Say, looks like this was already wrong in the original, or I may be
 reading this wrong, but it seems to me that the createDatabase() functions
 can leave an exclusive transaction active (if it was racing for the
 exclusive lock, and lost).

 You are right, it's not safe.  You're also right in that this is a
 separate bug (not a regression introduced in this branch), and I
 personally think we should stop creating the schema in the accessor
 anyway (and the old implementation should be deprecated too), and this
 error is quite unlikely to happen, so I was not sure if we really want
 to fix it, especially in this ticket.  Another reason I hesitated to
 do it is that it would be very difficult to test because "the error is
 unlike to happen".

 That said, I tried to fix it in the branch anyway (4df83ec).  But I
 couldn't come up with a reasonable test scenario, so I didn't add any
 test.  So, I'm okay either with or without incorporating this
 particular change to master.

 > Minor (heh) naming issue; perhaps the member variables version_ and
 minor_ should be renamed for consistency (to either major_ and minor_ or
 version_major_ and version_minor_)

 Yeah I had the same impression.  I still kept the inconsistent
 variable naming in the initial implementation so they match the DB
 column names. But now that you also pointed out the same thing, I
 changed the variable names.  (In any event they are internal
 variables, so we can easily change them).

 > Also perhaps we should set the major version in
 testdata/newschema.sqlite3  to something we never expect to actually reach
 (so that it won't fail on this test the moment we change schema version
 again :) ). Though of course it can be updated then as well. Come to think
 of it, all these db's need updating then. You were saying something about
 creating these in the tests themselves? :)

 As for newschema.sqlite3, yes, that's a good point.  I changed the
 version to 1000.  Regarding the need for updating test DB files in
 general, I think it's better to create test data at build/run time
 dynamically (except for the intentionally unusual/broken ones).  But
 I believe it's beyond the scope of this task and should go to a
 separate ticket (maybe one of "hardening" things).

 > changelog message final sentence should say 'files installed in the
 standard location have been' instead of 'has been' :)

 Ah, good catch, thanks.  I'll update the proposed text directly so I
 can simply copy-paste it later.

 > For the rest this looks OK to me.

 Okay, thanks.  I'm giving the ticket back to you as I made a few non
 trivial changes.

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