BIND 10 #3080: New DB backend: Postgres (patch)
BIND 10 Development
do-not-reply at isc.org
Tue Mar 25 15:28:08 UTC 2014
#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: 42 | Feature Depending on Ticket: 2592
| Add Hours to Ticket: 10
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tmark):
* hours: 32 => 10
* totalhours: 32 => 42
Comment:
As it stands there are build and runtime issues that need to be fixed
before
the review can continue. I am listing build and runtime issues first,
then
other issues:
BUILD ISSUES:
----------------------------------------------------------------------------
Using Postgres 9.3.2, I encountered build issues on both OS-X 10.9.2
(Mavericks) and Centos 6.4:
The value returned by pg_config used for LDFLAGS in configured begins with
"-L../../../src/common".
When building dhcpsrv, libtool fails as it cannot turn this
"../../../src/common" into an absolute path.
Under OS-X, the pg_config file defines LDFLAGS as this:
{{{
LDFLAGS = -L../../../src/common -L/usr/local/Cellar/ossp-uuid/1.6.2/lib
-Wl,-dead_strip_dylibs
}}}
under Centos, it is this:
{{{
LDFLAGS = -L../../../src/common -L/usr/lib64 -Wl,--as-needed
}}}
Removing "-L../../../src/common" from the Makfile and it builds without
error.
On Centos 6.4, configure fails to build Postgresql test code due to
missing
libraries, namely it cannot find libpq. By adding LIBDIR from pg_config
to
LDFLAGS in configure, this check passes.
I think the solution is to change configure.ac to use LIBDIR from
pg_config.
-----------------------------------------------------------------------
pgsql_lease_mgr.cc
Under clang++ on Mavericks:
pgsql_lease_mgr.cc:38:14: error: unused variable 'ADDRESS6_TEXT_MAX_LEN'
[-Werror,-Wunused-const-variable]
-----------------------------------------------------------------------
pgsql_lease_mgr.h
There are structures instantiations which such as the following:
{{{
PgSqlParam paddr4 = { .value = tmp.str() };
}}}
Not all compilers are going to like this, gcc 4.4.2 on Centos produces
this error:
{{{
pgsql_lease_mgr.cc:250: error: expected primary-expression before ‘.’
token
}}}
There dozens of occurances. I think it would be far more portable and far
cleaner to add constructors to the structs.
-------------------------------------------------------------------
There are several cppcheck complaints:
{{{
src/lib/dhcpsrv/pgsql_lease_mgr.cc:621: check_fail: The scope of the
variable 'r' can be reduced. (style,variableScope)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:47: check_fail: struct or union member
'TaggedStatement::index' is never used. (style,unusedStructMember)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:289: check_fail: Unused variable:
valid_lft_str (style,unusedVariable)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:300: check_fail: Unused variable:
subnet_id_str (style,unusedVariable)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:431: check_fail: Unused variable:
valid_lft_str (style,unusedVariable)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:887: check_fail: Unused variable: baddr
(style,unusedVariable)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:229: check_fail: Member variable
'PgSqlLease4Exchange::hwaddr_length_' is not initialized in the
constructor. (warning,uninitMemberVar)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:229: check_fail: Member variable
'PgSqlLease4Exchange::client_id_length_' is not initialized in the
constructor. (warning,uninitMemberVar)
src/lib/dhcpsrv/pgsql_lease_mgr.cc:395: check_fail: Member variable
'PgSqlLease6Exchange::duid_length_' is not initialized in the constructor.
(warning,uninitMemberVar)
src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc:117: check_fail: The
scope of the variable 'r' can be reduced. (style,variableScope)
src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc:139: check_fail: The
scope of the variable 'r' can be reduced. (style,variableScope)
}}}
-------------------------------------------------------------------
RUNTIME ISSUES:
-------------------------------------------------------------------
pgsql_lease_mgr.cc
Use of "SET AUTOCOMMIT TO OFF" is no longer supported and causes most of
the
unit tests to fail with this message:
{{{
STATEMENT: SET AUTOCOMMIT TO OFF
ERROR: SET AUTOCOMMIT TO OFF is no longer supported
}}}
It is currently being used in methods that fetch, such as
getLeaseCollection.
Since commits are meaningless for fetches there is no sense in doing it
even
if we could.
Similarly, fetch methods do not need the BEGIN and END statements. This is
an attempt to wrap the read in a transaction. Transactions should be
meaningless in terms of fetches. This should be examined and they should
be
removed unless there is a compelling reason to keep them.
----------------------------------------------------------------------
None of the PgSqlLeaseMgrTest tests pass under Mavericks/9.3.2. They all
fail on cltt comparisons:
{{{
[ RUN ] PgSqlLeaseMgrTest.basicLease4
WARNING: there is no transaction in progress
test_utils.cc:53: Failure
Value of: second->cltt_
Actual: 105456
Expected: first->cltt_
Which is: 123456
test_utils.cc:53: Failure
Value of: second->cltt_
Actual: 216567
Expected: first->cltt_
Which is: 234567
test_utils.cc:53: Failure
Value of: second->cltt_
Actual: 216567
Expected: first->cltt_
Which is: 234567
test_utils.cc:53: Failure
Value of: second->cltt_
Actual: 216567
Expected: first->cltt_
Which is: 234567
NOTICE: there is no transaction in progress
[ FAILED ] PgSqlLeaseMgrTest.basicLease4 (52 ms)
}}}
The issue is that calculation of cltt_ when extracting it from the
database
does not account for localtime. Note the actual and expected times are 5
hours apart and I am currently in EDT.
--------------------------------------------------------------------
Typcial 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
TBD
Conversion of hwaddr_ to string seems sketchy...
{{{
PgSqlParam pdest = { .value = string(lease_->hwaddr_.begin(),
lease_->hwaddr_.end()),
}}}
Seems like this should be treated the same way as client_id, I think it
should be this:
{{{
PgSqlParam pdest = { .value =
reinterpret_cast<char*>(&(lease_->hwaddr_[0]));
}}}
And of course ".value =" must be replaced as mentioned earlier.
----------------------------------------------------------------------
pgsql_lease_mgr.cc
Remove the "inline" from convertToQuery:
{{{
inline void
PgSqlLeaseMgr::convertToQuery(...
}}}
---------------------------------------------------------------------
pgsql_lease_mgr.cc
PgSqlLeaseExchange::stringToBool, the commentary should mention that
postgressql stores
booleans as NULL, 't', or 'f'. or call it pgBoolToBool().
-----------------------------------------------------------------
pgsql_lease_mgr.cc
Spacing is wrong on these:
{{{
PgSqlParam fqdn_fwd = { .value = lease_->fqdn_fwd_?"TRUE":"FALSE" };
}}}
-----------------------------------------------------------------
pgsql_lease_mgr.cc
This constant is actually wrong, the maximum number of parameters used SO
FAR,
is 13 used with UPDATE_LEASE6:
{{{
// Maximum number of parameters used in any signle query
const size_t MAX_PARAMETERS_IN_QUERY = 12;
}}}
This may explain why it TaggedStatement::types is dimensioned as as
MAX_PARAMETERS_IN_QUERY + 1.
There ought to be a cleaner way to do this but at the very least it should
be
accurate. There is no reason (at least none documented) for the " + 1".
-------------------------------------------------------------------
pgsql_lease_mgr.cc
I am not really sure I see any actual value in using the
BOOST_STATIC_ASSERTs,
and at the very least the asserts in MySqlLease6Exchange() and
PgSqlLease6Exchange() aren't even testing against the correct number.
They check that LEASE_COLUMNS is greater than 8, when in fact there are 12
column names:
{{{
PgSqlLease6Exchange() {
memset(duid_buffer_, 0, sizeof(duid_buffer_));
// Set the column names (for error messages)
columns_[0] = "address";
columns_[1] = "duid";
columns_[2] = "valid_lifetime";
columns_[3] = "expire";
columns_[4] = "subnet_id";
columns_[5] = "pref_lifetime";
columns_[6] = "lease_type";
columns_[7] = "iaid";
columns_[8] = "prefix_len";
columns_[9] = "fqdn_fwd";
columns_[10]= "fqdn_rev";
columns_[11]= "hostname";
BOOST_STATIC_ASSERT(8 < LEASE_COLUMNS);
params.reserve(LEASE_COLUMNS);
}
}}}
How much value is there in doing a static assert on two constants anyway?
----------------------------------------------------------------------
At this point I suspending further review, pending correction of the build
and
test failures.
As tomek is out of the office for two weeks, I will be making these fixes
myself.
--
Ticket URL: <https://bind10.isc.org/ticket/3080#comment:13>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list