Opened 4 years ago

Closed 3 years ago

#4276 closed enhancement (complete)

Common PostgreSQL Connector Pool

Reported by: tomek Owned by: tmark
Priority: medium Milestone: Kea1.1
Component: host-reservations Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 40 Add Hours to Ticket: 0.5
Total Hours: 23 Internal?: no

Description

The HostReservationDesign calls for implementing PostgreSQL host data source. Many of its operations will be common with PgsqlLeaseMgr?. Therefore it makes sense to split existing PgSQL LeaseMgr? into LeaseMgr? proper and a base class that's responsible for database connection and operations.

See #3681 for similar functionality already implemented for MySQL.

Subtickets

Attachments (1)

PostgresClasses.svg (39.9 KB) - added by tmark 3 years ago.
Postgresql Class Heirarchy

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by tmark

  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 3 years ago by tmark

Database connection operations were removed from PgSqlLeaseMgr into a new class, PgSqlConnection. A new wrapper class, PgSqlHolder, was added for the Postgresql API connection pointer, PGconn*.

A new base class, PgSqlExchange, was distilled from PgSqlLeaseExchange. This class houses common methods for marshalling data to and from Postgresql. A new wrapper class, PgSqlResult, was added for wrapping Postgresql API result sets, PGresult*.

The following diagram shows new class hierarchy:

Postgresql Class Heirarchy

As this is not visible to the user, no ChangeLog entry is needed.

comment:3 Changed 3 years ago by tmark

  • Add Hours to Ticket changed from 0 to 16
  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 16

Changed 3 years ago by tmark

Postgresql Class Heirarchy

comment:4 Changed 3 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:5 follow-up: Changed 3 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit a7a134e786562156073a6c2544ff08285aaf6170

These are very straight forward changes. Good idea with RAII wrapper. A couple nits follow.

src/lib/dhcpsrv/pgsql_connection.cc
You may want to think which of the included header files are really necessary. Probably all standard headers could be removed, so as static_assert.hpp.

src/lib/dhcpsrv/pgsql_connection.h
The PgSqlResult holds a pointer on which we call PQclear when it goes out of scope, so the object should probably be non-copyable to not cause a risk of clearing multiple times?

PgSqlHolder: constructor description says "Initialize PgSql". It is unclear what "PgSql" is in this context. Note that the constructor merely assigns NULL to pgconn_ object.

!PgSqlHolder::setConnection lacks any description.

src/lib/dhcpsrv/pgsql_exchange.cc
Now that the PgSqlExchange is hanging in its own header and .cc file, we should consider if some of the functions should have dedicated unit test. I think at least 'convertToDatabaseTime' and 'convertFromDatabaseTime' requires unit tests. Other functions would be tested indirectly by lease manager tests.

src/lib/dhcpsrv/pgsql_exchange.h
Did you really intend to define "PGSQL_EXCHANGE_MGR_H" or rather "PGSQL_EXCHANGE_H"?

In the description of PsqlBindArray: " Note that the data values are stored as pointers. These pointers need to valid for the duration of the PostgreSQL statement execution." there is a typo: "need to *be* valid".

Some of the functions like:

  • empty()
  • toText()
  • size()

could be const?

comment:6 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 16 to 2
  • Total Hours changed from 16 to 18

comment:7 in reply to: ↑ 5 Changed 3 years ago by tmark

Replying to marcin:

Reviewed commit a7a134e786562156073a6c2544ff08285aaf6170

These are very straight forward changes. Good idea with RAII wrapper. A couple nits follow.

Thanks. The RAII stuff was inspired by the MySql? side, so somebody else gets the credit.

src/lib/dhcpsrv/pgsql_connection.cc
You may want to think which of the included header files are really necessary. Probably all standard headers could be removed, so as static_assert.hpp.

Oops. I meant to go back and do this, good catch.

src/lib/dhcpsrv/pgsql_connection.h
The PgSqlResult holds a pointer on which we call PQclear when it goes out of scope, so the object should probably be non-copyable to not cause a risk of clearing multiple times?

Also meant to do that too. Again, good catch.

PgSqlHolder: constructor description says "Initialize PgSql". It is unclear what "PgSql" is in this context. Note that the constructor merely assigns NULL to pgconn_ object.

Got it.

!PgSqlHolder::setConnection lacks any description.

Man, I'm getting sloppy in my old age.

src/lib/dhcpsrv/pgsql_exchange.cc
Now that the PgSqlExchange is hanging in its own header and .cc file, we should consider if some of the functions should have dedicated unit test. I think at least 'convertToDatabaseTime' and 'convertFromDatabaseTime' requires unit tests. Other functions would be tested indirectly by lease manager tests.

I added tests for the time conversion functions.

src/lib/dhcpsrv/pgsql_exchange.h
Did you really intend to define "PGSQL_EXCHANGE_MGR_H" or rather "PGSQL_EXCHANGE_H"?

Gads. Fixed.

In the description of PsqlBindArray: " Note that the data values are stored as pointers. These pointers need to valid for the duration of the PostgreSQL statement execution." there is a typo: "need to *be* valid".

Some of the functions like:

  • empty()
  • toText()
  • size()

could be const?

Why yes they can be, Mr.Const. ;)

comment:8 Changed 3 years ago by tmark

  • Estimated Difficulty changed from 0 to 40
  • Owner changed from tmark to marcin
  • Total Hours changed from 18 to 20

comment:9 follow-up: Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 2 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 20 to 21

Reviewed commit ebfa39dc312e63ba690d2d41e770917b6ab45061

Because PgSqlExchange now becomes a common class, the unit tests should rather be placed in a separate .cc file, rather than unit tests for PgSqlLeaseMgr.

Also, the test declaration should rather be:

TEST(PgSqlExchangeTest, convertTimeTest)

instead of

TEST(PgSqlExchange, convertTimeTest)

I don't understand the problem with testing exact values of converted time. Here is my diff to the test that makes it check exact values, rather than using EXPECT_GT():

diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
index 8e6596d..219a856 100755
--- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
@@ -412,13 +412,18 @@ TEST(PgSqlExchange, convertTimeTest) {
     time_str = PgSqlExchange::convertToDatabaseTime(ref_time, 0);
     EXPECT_EQ(time_str, ref_time_str);
 
+    time_t ref_time2 = ref_time + 24 * 3600;
+    localtime_r(&ref_time2, &tinfo);
+    strftime(buffer, sizeof(buffer), "%Y-%m-%d %H:%M:%S", &tinfo);
+    ref_time_str.assign(buffer);
+
     // Add a day, we should get a string that's greater than the reference
     // string. Ok, maybe not the most exacting test, but you want I should
     // parse this?
     std::string time_str2;
     ASSERT_NO_THROW(time_str2 = PgSqlExchange::convertToDatabaseTime(ref_time,
                                                                      24*3600));
-    EXPECT_GT(time_str2, ref_time_str);
+    EXPECT_EQ(ref_time_str, time_str2);
 
     // Verify too large of a value is detected.
     ASSERT_THROW(PgSqlExchange::convertToDatabaseTime(DatabaseConnection::

comment:10 in reply to: ↑ 9 Changed 3 years ago by tmark

Replying to marcin:

Reviewed commit ebfa39dc312e63ba690d2d41e770917b6ab45061

Because PgSqlExchange now becomes a common class, the unit tests should rather be placed in a separate .cc file, rather than unit tests for PgSqlLeaseMgr.

Also, the test declaration should rather be:

TEST(PgSqlExchangeTest, convertTimeTest)

Done.

instead of

TEST(PgSqlExchange, convertTimeTest)

Done.

I don't understand the problem with testing exact values of converted time. Here is my diff to the test that makes it check exact values, rather than using EXPECT_GT():

diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
index 8e6596d..219a856 100755
--- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
+++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc
@@ -412,13 +412,18 @@ TEST(PgSqlExchange, convertTimeTest) {
     time_str = PgSqlExchange::convertToDatabaseTime(ref_time, 0);
     EXPECT_EQ(time_str, ref_time_str);
 
+    time_t ref_time2 = ref_time + 24 * 3600;
+    localtime_r(&ref_time2, &tinfo);
+    strftime(buffer, sizeof(buffer), "%Y-%m-%d %H:%M:%S", &tinfo);
+    ref_time_str.assign(buffer);
+
     // Add a day, we should get a string that's greater than the reference
     // string. Ok, maybe not the most exacting test, but you want I should
     // parse this?
     std::string time_str2;
     ASSERT_NO_THROW(time_str2 = PgSqlExchange::convertToDatabaseTime(ref_time,
                                                                      24*3600));
-    EXPECT_GT(time_str2, ref_time_str);
+    EXPECT_EQ(ref_time_str, time_str2);
 
     // Verify too large of a value is detected.
     ASSERT_THROW(PgSqlExchange::convertToDatabaseTime(DatabaseConnection::

I guess I didn't feel like repeating the buffer logic...so I made it a function instead.

comment:11 Changed 3 years ago by tmark

  • Owner changed from tmark to Marcin
  • Total Hours changed from 21 to 22

comment:12 Changed 3 years ago by marcin

  • Owner changed from Marcin to tmark

Your changes look good, and you're ok to merge it.

comment:13 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 1 to 0.5
  • Total Hours changed from 22 to 22.5

comment:14 Changed 3 years ago by tmark

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 22.5 to 23

Changes merged with git ff63173cf051769e5a020972e8c35a2c791186e4

Ticket is complete.

Note: See TracTickets for help on using tickets.