[4277] Addressed bulk of review comments

src/lib/dhcpsrv/tests/pgsql_exchange_unittest.cc
    - Added PgSqlBasicsTest test fixture class and tests which exercise all of
    the PostgreSQL data types we currently use with round-trip database writes
    and reads

src/lib/dhcpsrv/pgsql_connection.cc
src/lib/dhcpsrv/pgsql_connection.h
    - Moved PgSqlResult function impls from .h
    - Added exception safe implementation of getColumnLabel() to PgSqlResult

src/lib/dhcpsrv/pgsql_exchange.cc
src/lib/dhcpsrv/pgsql_exchange.h
    - PsqlBindArray::add() variants which accept raw pointers now throw
    if the pointer is NULL
    - PgSqlExchange::getColumnLabel() is now a wrapper around PgSqlResult method

src/lib/dhcpsrv/pgsql_host_data_source.h
src/lib/dhcpsrv/pgsql_host_data_source.cc
     - Commentary clean up

src/lib/dhcpsrv/pgsql_lease_mgr.cc
     - Commentary clean up

src/lib/dhcpsrv/pgsql_connection.cc

@@ -35,6 +35,59 @@ const int PGSQL_DEFAULT_CONNECTION_TIMEOUT = 5; // seconds
 
 const char PgSqlConnection::DUPLICATE_KEY[] = ERRCODE_UNIQUE_VIOLATION;
 
+PgSqlResult::PgSqlResult(PGresult *result) 
+    : result_(result), rows_(0), cols_(0) {
+    if (!result) {
+        isc_throw (BadValue, "PgSqlResult result pointer cannot be null");
+    }
+
+    rows_ = PQntuples(result);
+    cols_ = PQnfields(result);
+}
+
+void 
+PgSqlResult::rowCheck(int row) const {
+    if (row < 0 || row >= rows_) {
+        isc_throw (DbOperationError, "row: " << row 
+                   << ", out of range: 0.." << rows_);
+    }
+}
+
+PgSqlResult::~PgSqlResult() {
+    if (result_)  {
+        PQclear(result_);
+    }
+}
+
+void
+PgSqlResult::colCheck(int col) const {
+    if (col < 0 || col >= cols_) {
+        isc_throw (DbOperationError, "col: " << col
+                   << ", out of range: 0.." << cols_);
+    }
+}
+
+void
+PgSqlResult::rowColCheck(int row, int col) const {
+    rowCheck(row);
+    colCheck(col);
+}
+
+std::string
+PgSqlResult::getColumnLabel(const int col) const {
+    const char* label = NULL;
+    try {
+        colCheck(col);
+        label = PQfname(result_, col);
+    } catch (...) {
+        std::ostringstream os;
+        os << "Unknown column:" << col;
+        return (os.str());
+    }
+
+    return (label);
+}
+
 PgSqlTransaction::PgSqlTransaction(PgSqlConnection& conn)
     : conn_(conn), committed_(false) {
     conn_.startTransaction();

src/lib/dhcpsrv/pgsql_connection.h

@@ -28,7 +28,6 @@ const size_t PGSQL_MAX_PARAMETERS_IN_QUERY = 32;
 /// Each statement is associated with an index, which is used to reference the
 /// associated prepared statement.
 struct PgSqlTaggedStatement {
-
     /// Number of parameters for a given query
     int nbparams;
 
@@ -48,16 +47,16 @@ struct PgSqlTaggedStatement {
 
 /// @brief Constants for PostgreSQL data types
 /// This are defined by PostreSQL in <catalog/pg_type.h>, but including
-/// this file is extrordinarily convoluted, so we'll use these to fill-in.
+/// this file is extraordinarily convoluted, so we'll use these to fill-in.
 const size_t OID_NONE = 0;   // PostgreSQL infers proper type
 const size_t OID_BOOL = 16;
 const size_t OID_BYTEA = 17;
 const size_t OID_INT8 = 20;  // 8 byte int
-const size_t OID_INT4 = 23;  // 4 byte int
 const size_t OID_INT2 = 21;  // 2 byte int
+const size_t OID_INT4 = 23;  // 4 byte int
 const size_t OID_TEXT = 25;
-const size_t OID_TIMESTAMP = 1114;
 const size_t OID_VARCHAR = 1043;
+const size_t OID_TIMESTAMP = 1114;
 
 //@}
 
@@ -85,23 +84,12 @@ public:
     /// Store the pointer to the result set to being fetched.  Set row
     /// and column counts for convenience.
     ///
-    PgSqlResult(PGresult *result) : result_(result), rows_(0), cols_(0) {
-        if (!result) {
-            isc_throw (BadValue, "PgSqlResult result pointer cannot be null");
-        }
-
-        rows_ = PQntuples(result);
-        cols_ = PQnfields(result);
-    }
+    PgSqlResult(PGresult *result);
 
     /// @brief Destructor
     ///
     /// Frees the result set
-    ~PgSqlResult() {
-        if (result_)  {
-            PQclear(result_);
-        }
-    }
+    ~PgSqlResult();
 
     /// @brief Returns the number of rows in the result set.
     int getRows() const {
@@ -117,35 +105,34 @@ public:
     ///
     /// @param row index to range check
     ///
-    /// @throw throws DbOperationError if the row index is out of range
-    void rowCheck(int row) const {
-        if (row >= rows_) {
-            isc_throw (DbOperationError, "row: " << row << ", out of range: 0.." << rows_);
-        }
-    }
+    /// @throw DbOperationError if the row index is out of range
+    void rowCheck(int row) const;
 
     /// @brief Determines if a column index is valid
     ///
     /// @param col index to range check
     ///
-    /// @throw throws DbOperationError if the column index is out of range
-    void colCheck(int col) const {
-        if (col >= cols_) {
-            isc_throw (DbOperationError, "col: " << col << ", out of range: 0.." << cols_);
-        }
-    }
+    /// @throw DbOperationError if the column index is out of range
+    void colCheck(int col) const;
 
     /// @brief Determines if both a row and column index are valid
     ///
     /// @param row index to range check
     /// @param col index to range check
     ///
-    /// @throw throws DbOperationError if either the row or column index
+    /// @throw DbOperationError if either the row or column index
     /// is out of range
-    void rowColCheck(int row, int col) const {
-        rowCheck(row);
-        colCheck(col);
-    }
+    void rowColCheck(int row, int col) const;
+
+    /// @brief Fetches the name of the column in a result set
+    ///
+    /// Returns the column name of the column from the result set.
+    /// If the column index is out of range it will return the
+    /// string "Unknown column:<index>"
+    ///
+    /// @param col index of the column name to fetch
+    /// @return string containing the name of the column
+    std::string getColumnLabel(const int col) const;
 
     /// @brief Conversion Operator
     ///
@@ -275,7 +262,7 @@ public:
     /// @brief Commits transaction.
     ///
     /// Commits all changes made during the transaction by executing the
-    /// SQL statement: "COMMIT">
+    /// SQL statement: "COMMIT"
     ///
     /// @throw DbOperationError if statement execution fails
     void commit();
@@ -410,8 +397,6 @@ public:
 
 };
 
-
-
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
 

src/lib/dhcpsrv/pgsql_exchange.cc

@@ -21,6 +21,10 @@ const char* PsqlBindArray::TRUE_STR = "TRUE";
 const char* PsqlBindArray::FALSE_STR = "FALSE";
 
 void PsqlBindArray::add(const char* value) {
+    if (!value) {
+        isc_throw(BadValue, "PsqlBindArray::add - char* value cannot be NULL");
+    }
+
     values_.push_back(value);
     lengths_.push_back(strlen(value));
     formats_.push_back(TEXT_FMT);
@@ -39,6 +43,10 @@ void PsqlBindArray::add(const std::vector<uint8_t>& data) {
 }
 
 void PsqlBindArray::add(const uint8_t* data, const size_t len) {
+    if (!data) {
+        isc_throw(BadValue, "PsqlBindArray::add - uint8_t data cannot be NULL");
+    }
+
     values_.push_back(reinterpret_cast<const char*>(&(data[0])));
     lengths_.push_back(len);
     formats_.push_back(BINARY_FMT);
@@ -70,7 +78,11 @@ void PsqlBindArray::addNull(const int format) {
     formats_.push_back(format);
 }
 
-// Eventually this could replace add(std::string&) ?
+/// @todo Eventually this could replace add(std::string&)? This would mean
+/// all bound strings would be internally copies rather than perhaps belonging
+/// to the originating object such as Host::hostname_.  One the one hand it
+/// would make all strings handled one-way only, on the other hand it would
+/// mean duplicating strings where it isn't strictly necessary.
 void PsqlBindArray::addTempString(const std::string& str) {
     bound_strs_.push_back(ConstStringPtr(new std::string(str)));
     PsqlBindArray::add((bound_strs_.back())->c_str());
@@ -246,15 +258,7 @@ PgSqlExchange::convertFromBytea(const PgSqlResult& r, const int row,
 
 std::string
 PgSqlExchange::getColumnLabel(const PgSqlResult& r, const size_t column) {
-    r.colCheck(column);
-    const char* label = PQfname(r, column);
-    if (!label) {
-        std::ostringstream os;
-        os << "Unknown column:" << column;
-        return (os.str());
-    }
-
-    return (label);
+    return (r.getColumnLabel(column));
 }
 
 std::string 
@@ -264,7 +268,7 @@ PgSqlExchange::dumpRow(const PgSqlResult& r, int row) {
     int columns = r.getCols();
     for (int col = 0; col < columns; ++col) {
         const char* val = getRawColumnValue(r, row, col);
-        std::string name = getColumnLabel(r, col);
+        std::string name = r.getColumnLabel(col);
         int format = PQfformat(r, col); 
 
         stream << col << "   " << name << " : " ;

src/lib/dhcpsrv/pgsql_exchange.h

@@ -82,6 +82,7 @@ struct PsqlBindArray {
     /// remains in scope until the bind array has been discarded.
     ///
     /// @param value char array containing the null-terminated text to add.
+    /// @throw DbOperationError if value is NULL.
     void add(const char* value);
 
     /// @brief Adds an string value to the bind array
@@ -113,6 +114,7 @@ struct PsqlBindArray {
     ///
     /// @param data buffer of binary data.
     /// @param len  number of bytes of data in buffer
+    /// @throw DbOperationError if data is NULL.
     void add(const uint8_t* data, const size_t len);
 
     /// @brief Adds a boolean value to the bind array.
@@ -159,7 +161,7 @@ struct PsqlBindArray {
     /// @brief Binds a the given string to the bind array.
     ///
     /// Prior to added the The given string the vector of exchange values,
-    /// it duplicated as a ConstStringPtr and saved internally.  This garauntees
+    /// it duplicated as a ConstStringPtr and saved internally.  This guarantees
     /// the string remains in scope until the PsqlBindArray is destroyed,
     /// without the caller maintaining the string values.
     ///
@@ -262,7 +264,15 @@ public:
     static const char* getRawColumnValue(const PgSqlResult& r, const int row,
                                          const size_t col);
 
-    /// @todo
+    /// @brief Fetches the name of the column in a result set
+    ///
+    /// Returns the column name of the column from the result set.
+    /// If the column index is out of range it will return the
+    /// string "Unknown column:<index>".  Note this is NOT from the
+    /// list of columns defined in the exchange.
+    ///
+    /// @param col index of the column name to fetch
+    /// @return string containing the name of the column
     static std::string getColumnLabel(const PgSqlResult& r, const size_t col);
 
     /// @brief Fetches text column value as a string
@@ -319,6 +329,8 @@ public:
     /// @param r the result set containing the query results
     /// @param row the row number within the result set
     /// @param col the column number within the row
+    ///
+    /// @return True if the column values in the row is NULL, false otherwise.
     static bool isColumnNull(const PgSqlResult& r, const int row,
                              const size_t col);
 
@@ -372,6 +384,8 @@ public:
     ///
     /// @param r the result set containing the query results
     /// @param row the row number within the result set
+    ///
+    /// @return A string depiction of the row contents.
     static std::string dumpRow(const PgSqlResult& r, int row);
 
 protected:

src/lib/dhcpsrv/pgsql_host_data_source.cc

@@ -34,6 +34,8 @@ using namespace std;
 namespace {
 
 /// @brief Maximum length of option value.
+/// The maximum size of the raw option data that may be read from the
+/// database. 
 const size_t OPTION_VALUE_MAX_LEN = 4096;
 
 /// @brief Numeric value representing last supported identifier.
@@ -106,7 +108,7 @@ public:
     /// @brief Reinitializes state information
     ///
     /// This function should be called in between statement executions.
-    /// Deriving classes should inovke this method as well as be reset
+    /// Deriving classes should invoke this method as well as be reset
     /// all of their own stateful values.
     virtual void clear() {
         host_.reset();
@@ -375,13 +377,30 @@ private:
         /// @brief Creates instance of the currently processed option.
         ///
         /// This method detects if the currently processed option is a new
-        /// instance. It makes it determination by comparing the identifier
+        /// instance. It makes its determination by comparing the identifier
         /// of the currently processed option, with the most recently processed
         /// option. If the current value is greater than the id of the recently
         /// processed option it is assumed that the processed row holds new
         /// option information. In such case the option instance is created and
         /// inserted into the configuration passed as argument.
         ///
+        /// This logic is necessary to deal with result sets made from multiple
+        /// left joins which contain duplicated data.  For instance queries
+        /// returning both v4 and v6 options for a host would generate result
+        /// sets similar to this:
+        /// @code
+        ///
+        /// row 0: host-1  v4-opt-1  v6-opt-1
+        /// row 1: host-1  v4-opt-1  v6-opt-2
+        /// row 2: host-1  v4-opt-1  v6-opt-3
+        /// row 4: host-1  v4-opt-2  v6-opt-1
+        /// row 5: host-1  v4-opt-2  v6-opt-2
+        /// row 6: host-1  v4-opt-2  v6-opt-3
+        /// row 7: host-2  v4-opt-1  v6-opt-1
+        /// row 8: host-2  v4-opt-2  v6-opt-1
+        ///  :
+        /// @endcode
+        ///
         /// @param cfg Pointer to the configuration object into which new
         /// option instances should be inserted.
         /// @param r result set containing one or more rows from a dhcp
@@ -421,8 +440,6 @@ private:
                                             sizeof(value), value_len);
 
             // formatted_value: TEXT
-            // @todo Should we attempt to enforce max value of 8K?
-            // If so, we should declare this VARCHAR[8K] in the table
             std::string formatted_value;
             PgSqlExchange::getColumnValue(r, row, formatted_value_index_,
                                           formatted_value);
@@ -596,7 +613,7 @@ public:
     /// @brief Clears state information
     ///
     /// This function should be called in between statement executions.
-    /// Deriving classes should inovke this method as well as be reset
+    /// Deriving classes should invoke this method as well as be reset
     /// all of their own stateful values.
     virtual void clear() {
         PgSqlHostExchange::clear();
@@ -611,10 +628,11 @@ public:
 
     /// @brief Processes the current row.
     ///
-    /// The processed row includes both host information and DHCP option
-    /// information. Because used SELECT query use LEFT JOIN clause, the
-    /// some rows contain duplicated host or options entries. This method
-    /// detects duplicated information and discards such entries.
+    /// The fetched row includes both host information and DHCP option
+    /// information. Because the SELECT queries use one or more LEFT JOIN
+    /// clauses, the result set may contain duplicated host or options 
+    /// entries. This method detects duplicated information and discards such 
+    /// entries.
     ///
     /// @param [out] hosts Container holding parsed hosts and options.
     virtual void processRowData(ConstHostCollection& hosts,
@@ -685,7 +703,7 @@ private:
 /// host information, DHCPv4 options, DHCPv6 options and IPv6 reservations.
 ///
 /// This class extends the @ref PgSqlHostWithOptionsExchange class with the
-/// mechanisms to retrieve IPv6 reservations. This class is used in sitations
+/// mechanisms to retrieve IPv6 reservations. This class is used in situations
 /// when it is desired to retrieve DHCPv6 specific information about the host
 /// (DHCPv6 options and reservations), or entire information about the host
 /// (DHCPv4 options, DHCPv6 options and reservations). The following are the
@@ -727,7 +745,7 @@ public:
     /// @brief Reinitializes state information
     ///
     /// This function should be called in between statement executions.
-    /// Deriving classes should inovke this method as well as be reset
+    /// Deriving classes should invoke this method as well as be reset
     /// all of their own stateful values.
     void clear() {
         PgSqlHostWithOptionsExchange::clear();
@@ -921,7 +939,7 @@ public:
             bind_array->add(resv.getPrefixLen());
 
             // type: SMALLINT NOT NULL
-            // See lease6_types for values (0 = IA_NA, 1 = IA_TA, 2 = IA_PD)
+            // See lease6_types table for values (0 = IA_NA, 2 = IA_PD)
             uint16_t type = resv.getType() == IPv6Resrv::TYPE_NA ? 0 : 2;
             bind_array->add(type);
 
@@ -1089,8 +1107,8 @@ public:
     enum StatementIndex {
         INSERT_HOST,            // Insert new host to collection
         INSERT_V6_RESRV,        // Insert v6 reservation
-        INSERT_V4_HOST_OPTION,       // Insert DHCPv4 option
-        INSERT_V6_HOST_OPTION,       // Insert DHCPv6 option
+        INSERT_V4_HOST_OPTION,  // Insert DHCPv4 option
+        INSERT_V6_HOST_OPTION,  // Insert DHCPv6 option
         GET_HOST_DHCPID,        // Gets hosts by host identifier
         GET_HOST_ADDR,          // Gets hosts by IPv4 address
         GET_HOST_SUBID4_DHCPID, // Gets host by IPv4 SubnetID, HW address/DUID
@@ -1122,7 +1140,7 @@ public:
     /// of a single row with one column, the value of the primary key.
     /// Defaults to false.
     ///
-    /// @returns 0 if return_last_id is false, otherwise it returns the
+    /// @return 0 if return_last_id is false, otherwise it returns the
     /// the value in the result set in the first col of the first row.
     ///
     /// @throw isc::dhcp::DuplicateEntry Database throws duplicate entry error
@@ -1245,8 +1263,9 @@ public:
 /// @brief Prepared MySQL statements used by the backend to insert and
 /// retrieve hosts from the database.
 PgSqlTaggedStatement tagged_statements[] = {
+    // PgSqlHostDataSourceImpl::INSERT_HOST
     // Inserts a host into the 'hosts' table. Returns the inserted host id.
-    {8, // PgSqlHostDataSourceImpl::INSERT_HOST,
+    {8, 
      { OID_BYTEA, OID_INT2,
        OID_INT4, OID_INT4, OID_INT8, OID_VARCHAR,
        OID_VARCHAR, OID_VARCHAR },
@@ -1257,8 +1276,9 @@ PgSqlTaggedStatement tagged_statements[] = {
      "VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING host_id"
     },
 
+    //PgSqlHostDataSourceImpl::INSERT_V6_RESRV
     // Inserts a single IPv6 reservation into 'reservations' table.
-    {5, //PgSqlHostDataSourceImpl::INSERT_V6_RESRV,
+    {5, 
      { OID_VARCHAR, OID_INT2, OID_INT4, OID_INT4, OID_INT4 },
      "insert_v6_resrv",
      "INSERT INTO ipv6_reservations(address, prefix_len, type, "
@@ -1266,9 +1286,10 @@ PgSqlTaggedStatement tagged_statements[] = {
      "VALUES ($1, $2, $3, $4, $5)"
     },
 
+    // PgSqlHostDataSourceImpl::INSERT_V4_HOST_OPTION
     // Inserts a single DHCPv4 option into 'dhcp4_options' table.
     // Using fixed scope_id = 3, which associates an option with host.
-    {6, // PgSqlHostDataSourceImpl::INSERT_V4_HOST_OPTION,
+    {6, 
      { OID_INT2, OID_BYTEA, OID_TEXT,
        OID_VARCHAR, OID_BOOL, OID_INT8},
      "insert_v4_host_option",
@@ -1277,9 +1298,10 @@ PgSqlTaggedStatement tagged_statements[] = {
      "VALUES ($1, $2, $3, $4, $5, $6, 3)"
     },
 
+    // PgSqlHostDataSourceImpl::INSERT_V6_HOST_OPTION
     // Inserts a single DHCPv6 option into 'dhcp6_options' table.
     // Using fixed scope_id = 3, which associates an option with host.
-    {6, // PgSqlHostDataSourceImpl::INSERT_V6_HOST_OPTION,
+    {6,
      { OID_INT2, OID_BYTEA, OID_TEXT,
        OID_VARCHAR, OID_BOOL, OID_INT8},
      "insert_v6_host_option",
@@ -1288,11 +1310,12 @@ PgSqlTaggedStatement tagged_statements[] = {
      "VALUES ($1, $2, $3, $4, $5, $6, 3)"
     },
 
+    // PgSqlHostDataSourceImpl::GET_HOST_DHCPID
     // Retrieves host information, IPv6 reservations and both DHCPv4 and
     // DHCPv6 options associated with the host. The LEFT JOIN clause is used
     // to retrieve information from 4 different tables using a single query.
     // Hence, this query returns multiple rows for a single host.
-    {2, // PgSqlHostDataSourceImpl::GET_HOST_DHCPID,
+    {2, 
      { OID_BYTEA, OID_INT2 },
      "get_host_dhcpid",
      "SELECT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
@@ -1311,10 +1334,11 @@ PgSqlTaggedStatement tagged_statements[] = {
      "ORDER BY h.host_id, o4.option_id, o6.option_id, r.reservation_id"
     },
 
+    // PgSqlHostDataSourceImpl::GET_HOST_ADDR
     // Retrieves host information along with the DHCPv4 options associated with
     // it. Left joining the dhcp4_options table results in multiple rows being
     // returned for the same host. The host is retrieved by IPv4 address.
-    { 1, // PgSqlHostDataSourceImpl::GET_HOST_ADDR,
+    {1,
      { OID_INT8 }, "get_host_addr",
      "SELECT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
      "  h.dhcp4_subnet_id, h.dhcp6_subnet_id, h.ipv4_address, h.hostname, "
@@ -1326,10 +1350,11 @@ PgSqlTaggedStatement tagged_statements[] = {
      "ORDER BY h.host_id, o.option_id"
     },
 
+    //PgSqlHostDataSourceImpl::GET_HOST_SUBID4_DHCPID
     // Retrieves host information and DHCPv4 options using subnet identifier
     // and client's identifier. Left joining the dhcp4_options table results in
     // multiple rows being returned for the same host.
-    { 3, //PgSqlHostDataSourceImpl::GET_HOST_SUBID4_DHCPID,
+    {3,
      { OID_INT4, OID_INT2, OID_BYTEA },
      "get_host_subid4_dhcpid",
      "SELECT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
@@ -1343,10 +1368,11 @@ PgSqlTaggedStatement tagged_statements[] = {
      "ORDER BY h.host_id, o.option_id"
     },
 
+    //PgSqlHostDataSourceImpl::GET_HOST_SUBID6_DHCPID
     // Retrieves host information, IPv6 reservations and DHCPv6 options
     // associated with a host. The number of rows returned is a multiplication
     // of number of IPv6 reservations and DHCPv6 options.
-    {3, //PgSqlHostDataSourceImpl::GET_HOST_SUBID6_DHCPID,
+    {3,
      { OID_INT4, OID_INT2, OID_BYTEA },
      "get_host_subid6_dhcpid",
      "SELECT h.host_id, h.dhcp_identifier, "
@@ -1364,11 +1390,12 @@ PgSqlTaggedStatement tagged_statements[] = {
      "ORDER BY h.host_id, o.option_id, r.reservation_id"
     },
 
+    //PgSqlHostDataSourceImpl::GET_HOST_SUBID_ADDR
     // Retrieves host information and DHCPv4 options for the host using subnet
     // identifier and IPv4 reservation. Left joining the dhcp4_options table
     // results in multiple rows being returned for the host. The number of
     // rows depends on the number of options defined for the host.
-    { 2,  //PgSqlHostDataSourceImpl::GET_HOST_SUBID_ADDR,
+    {2,
      { OID_INT4, OID_INT8 },
      "get_host_subid_addr",
      "SELECT h.host_id, h.dhcp_identifier, h.dhcp_identifier_type, "
@@ -1381,13 +1408,14 @@ PgSqlTaggedStatement tagged_statements[] = {
      "ORDER BY h.host_id, o.option_id"
     },
 
+    // PgSqlHostDataSourceImpl::GET_HOST_PREFIX
     // Retrieves host information, IPv6 reservations and DHCPv6 options
     // associated with a host using prefix and prefix length. This query
     // returns host information for a single host. However, multiple rows
     // are returned due to left joining IPv6 reservations and DHCPv6 options.
     // The number of rows returned is multiplication of number of existing
     // IPv6 reservations and DHCPv6 options.
-    {2, // PgSqlHostDataSourceImpl::GET_HOST_PREFIX,
+    {2, 
      { OID_VARCHAR, OID_INT2 },
      "get_host_prefix",
      "SELECT h.host_id, h.dhcp_identifier, "
@@ -1407,8 +1435,9 @@ PgSqlTaggedStatement tagged_statements[] = {
      "ORDER BY h.host_id, o.option_id, r.reservation_id"
     },
 
+    //PgSqlHostDataSourceImpl::GET_VERSION
     // Retrieves MySQL schema version.
-    { 0, //PgSqlHostDataSourceImpl::GET_VERSION,
+    {0, 
      { OID_NONE },
      "get_version",
      "SELECT version, minor FROM schema_version"
@@ -1460,13 +1489,13 @@ PgSqlHostDataSourceImpl::addStatement(StatementIndex stindex,
     int s = PQresultStatus(r);
 
     if (s != PGRES_COMMAND_OK) {
-        // Failure: check for the special case of duplicate entry.  If this is
-        // the case, we return false to indicate that the row was not added.
-        // Otherwise we throw an exception.
+        // Failure: check for the special case of duplicate entry.
         if (conn_.compareError(r, PgSqlConnection::DUPLICATE_KEY)) {
             isc_throw(DuplicateEntry, "Database duplicate entry error");
         }
 
+        // Connection determines if the error is fatal or not, and
+        // throws the appropriate exception
         conn_.checkStatementError(r, tagged_statements[stindex]);
     }
 
@@ -1748,6 +1777,11 @@ PgSqlHostDataSource::get4(const SubnetID& subnet_id,
 ConstHostPtr
 PgSqlHostDataSource::get4(const SubnetID& subnet_id,
                           const asiolink::IOAddress& address) const {
+    if (!address.isV4()) {
+        isc_throw(BadValue, "PgSqlHostDataSource::get4(id, address) - "
+                  " wrong address type, address supplied is an IPv6 address");
+    }
+
     // Set up the WHERE clause value
     PsqlBindArrayPtr bind_array(new PsqlBindArray());
 

src/lib/dhcpsrv/pgsql_host_data_source.h

@@ -22,6 +22,16 @@ class PgSqlHostDataSourceImpl;
 /// This class implements the @ref isc::dhcp::BaseHostDataSource interface to
 /// the PostgreSQL database. Use of this backend presupposes that a PostgreSQL
 /// database is available and that the Kea schema has been created within it.
+///
+/// Reservations are uniquely identified by identifier type and value. Currently
+/// The currently supported values are defined in @ref Host::IdentifierType
+/// as well as in host_identifier_table:
+///
+/// - IDENT_HWADDR
+/// - IDENT_DUID
+/// - IDENT_CIRCUIT_ID
+/// - IDENT_CLIENT_ID
+///
 class PgSqlHostDataSource: public BaseHostDataSource {
 public:
 
@@ -50,6 +60,8 @@ public:
     PgSqlHostDataSource(const DatabaseConnection::ParameterMap& parameters);
 
     /// @brief Virtual destructor.
+    /// Frees database resources and closes the database connection through
+    /// the destruction of member impl_.
     virtual ~PgSqlHostDataSource();
 
     /// @brief Return all hosts for the specified HW address or DUID.
@@ -145,13 +157,11 @@ public:
     /// if this address is not reserved for some other host and do not allocate
     /// this address if reservation is present.
     ///
-    /// Implementations of this method should guard against invalid addresses,
-    /// such as IPv6 address.
-    ///
     /// @param subnet_id Subnet identifier.
     /// @param address reserved IPv4 address.
     ///
     /// @return Const @c Host object using a specified IPv4 address.
+    /// @throw BadValue is given an IPv6 address
     virtual ConstHostPtr
     get4(const SubnetID& subnet_id, const asiolink::IOAddress& address) const;
 
@@ -198,30 +208,51 @@ public: