BIND 10 #3080: New DB backend: Postgres (patch)

[BIND 10 Development]

#3080: New DB backend: Postgres (patch)
-------------------------------------+-------------------------------------
            Reporter:  dclink        |                        Owner:  tmark
                Type:  enhancement   |                       Status:
            Priority:  medium        |  reviewing
           Component:  dhcpdb        |                    Milestone:  DHCP-
            Keywords:                |  Kea0.9-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  3             |              Defect Severity:  N/A
         Total Hours:  50            |  Feature Depending on Ticket:  2592
                                     |          Add Hours to Ticket:  8
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * hours:  10 => 8
 * totalhours:  42 => 50


Comment:

 I have addressed most of the above issues with
 git commits efbde0adb7a7dcff8438fc7a5209afe77b4b1b32 and
 6adb8b731be3a72e9a6e407ca076a7ee1810d826

 With these changes it should build and pass unit testing on OS-X and
 Centos
 with Postgres9.3.4. There are still runtime issues to address.  I have run
 some basic manual tests on Centos and verified v4 and v6 leases can be
 stored
 and retrieved.

 Changed the definition of PGSQL_LIBS for building Kea with PostgreSQL
 back end to use pg_config value for LIBDIR rather than LDFLAGS.  The
 latter
 did not build with PostgreSQL 9.3.4 on OS-X or Centos.

 Added OS-X version numbers 10.9.1 and 10.9.2 to the test for setting the
 value
 of bind10_undefined_pthread_behavior.  Without this the death test for
 conditional variables fails as the problem introduced in 10.9 is still
 there
 as of 10.9.2.  This is unrelated to PostgreSQL.

 Changed expire column type in lease tables to be "TIMESTAMP WITH TIME
 ZONE",
 and added methods to convert to and from such fields to LeaseExchange.
 This
 corrects mismatched time conversion to and from database which was causing
 unit tests to fail.

 Added constructors to PgSqlParam to eliminate use of ".value" initializers
 and
 to provide a safe, uniform way to create parameters for binary data. Prior
 to
 this valgrind was reporting invalid reads when vectors were statically
 cast
 to char*.

 Removed superfluous BOOST_STATIC_ASSERT and corrected values tested in
 remaining
 check.

 Removed use of "SET AUTOCOMMIT TO" as it is no longer supported in
 PostgreSQL.

 Altered failure logic in PgSqlLeaseMgr::openDatabase() to release
 connection
 if it is not NULL. This was causing memory leak in unit tests.

 Added PQfinish call to createSchema() function to release the connection
 to fix
 memory leaks during unit testing.

 Cleaned most cppcheck complaints.


 Outstanding Issues:

 RUNTIME ISSUE:

 -------------------------------------------------------------------

 Several methods are structured as a series of sql statement executes and
 are therefore wrapped in a transaction by executing SQL "BEGIN" and "END"
 statements.  The most pressing issue with this that these methods do not
 properly clean up if errors occur.  Rather they throw and therefore bypass
 executing the "END" or attempting a rollback.  This leaves the current
 transaction open and all subsequent transactions fail.  The only way to
 recover is to restart the server.

 In addition to error handling problem, structuring these methods as
 multiple
 statement executions is going to be costly from a performance stand point.
 Their counterparts in the MySQL implementation are performed as single
 statements.

 At the very least, we need to alter the error handling to clean up, but a
 better approach would be to do away with need for multiple statement
 executions.

 ----------------------------------------------------------------------

 Minor Review Issues:

 --------------------------------------------------------------------
 pgsql_lease_mgr.cc

 The Oid integer values need to be replaced either with constants we define
 or
 use the #defines in server/catalog/pg_type.h.  However, including
 pg_type.h
 does come with a twist, you need to include a higher level include such as
 server/postgres_fe.h but that introduces some redefines.  It can be
 circumvented as follows:

 {{{
 // Undefine the following from our config.h so they do not collide
 // with those in postgres_fe.h.
 #undef PACKAGE_BUGREPORT
 #undef PACKAGE_NAME
 #undef PACKAGE_STRING
 #undef PACKAGE_TARNAME
 #undef PACKAGE_VERSION

 #include <postgres_fe.h>
 #include <catalog/pg_type.h>
 }}}

 -------------------------------------------------------------------

 pgsql_lease_mgr.h

 The typedef "bindparams" should not be lowercased.

 ----------------------------------------------------------------------
 pgsql_lease_mgr.cc

 Several comments still refer to MYSQL_BIND

 ------------------------------------------------------------------------
 pgsql_lease_mgr.cc

 Remove the "inline" from convertToQuery:

 {{{
 inline void
 PgSqlLeaseMgr::convertToQuery(...
 }}}

 ---------------------------------------------------------------------

 I have not yet reviewed the unit tests themselves for completeness.

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