From f65d1a473e3a07a90e7bf6ddb96816177f185cd2 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Wed, 11 Sep 2019 18:15:47 +0300 Subject: [PATCH 01/12] [#886, !508] added thread resource mgr --- src/lib/dhcpsrv/Makefile.am | 1 + src/lib/dhcpsrv/tests/Makefile.am | 1 + .../tests/thread_resource_mgr_unittest.cc | 80 +++++++++++++++++++ src/lib/dhcpsrv/thread_resource_mgr.h | 49 ++++++++++++ 4 files changed, 131 insertions(+) create mode 100644 src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc create mode 100644 src/lib/dhcpsrv/thread_resource_mgr.h diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am index d0076959dd..1974c9a9b2 100644 --- a/src/lib/dhcpsrv/Makefile.am +++ b/src/lib/dhcpsrv/Makefile.am @@ -118,6 +118,7 @@ libkea_dhcpsrv_la_SOURCES += lease_mgr_factory.cc lease_mgr_factory.h libkea_dhcpsrv_la_SOURCES += memfile_lease_mgr.cc memfile_lease_mgr.h libkea_dhcpsrv_la_SOURCES += memfile_lease_storage.h libkea_dhcpsrv_la_SOURCES += multi_threading_utils.h multi_threading_utils.cc +libkea_dhcpsrv_la_SOURCES += thread_resource_mgr.h if HAVE_MYSQL libkea_dhcpsrv_la_SOURCES += mysql_lease_mgr.cc mysql_lease_mgr.h diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index d9cac94776..8684f6379e 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -124,6 +124,7 @@ libdhcpsrv_unittests_SOURCES += shared_networks_list_parser_unittest.cc libdhcpsrv_unittests_SOURCES += srv_config_unittest.cc libdhcpsrv_unittests_SOURCES += subnet_unittest.cc libdhcpsrv_unittests_SOURCES += test_get_callout_handle.cc test_get_callout_handle.h +libdhcpsrv_unittests_SOURCES += thread_resource_mgr_unittest.cc libdhcpsrv_unittests_SOURCES += triplet_unittest.cc libdhcpsrv_unittests_SOURCES += test_utils.cc test_utils.h libdhcpsrv_unittests_SOURCES += timer_mgr_unittest.cc diff --git a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc new file mode 100644 index 0000000000..5561567a3a --- /dev/null +++ b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc @@ -0,0 +1,80 @@ +// Copyright (C) 2018-2019 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include + +#include + +#include + +#include + +using namespace isc::dhcp; +using namespace std; + +namespace { + +/// @brief Test Fixture for testing isc::dhcp::ThreadResourceMgr +class ThreadResourceMgrTest : public ::testing::Test { +}; + +template +class Resource { +public: + Resource() { + lock_guard lk(mutex_); + Resource::count_++; + Resource::created_count_++; + } + + virtual ~Resource() { + lock_guard lk(mutex_); + Resource::count_--; + Resource::destroyed_count_++; + } + + static uint32_t count() { + lock_guard lk(mutex_); + return Resource::count_; + } + + static uint32_t createdCount() { + lock_guard lk(mutex_); + return Resource::created_count_; + } + + static uint32_t destroyedCount() { + lock_guard lk(mutex_); + return Resource::destroyed_count_; + } + + static void reset() { + lock_guard lk(mutex_); + Resource::count_ = 0; + Resource::created_count_ = 0; + Resource::destroyed_count_ = 0; + } +private: + /// @brief total number of instances at any given time + static uint32_t count_; + + /// @brief total number of instances ever created + static uint32_t created_count_; + + /// @brief total number of instances ever destroyed + static uint32_t destroyed_count_; + + /// @brief mutex used to keep the internal state consistent + static std::mutex mutex_; +}; + +// This test verifies that each thread can access it's own allocated resource +TEST(ThreadResourceMgrTest, testThreadResources) { + ThreadResourceMgr integers; + ThreadResourceMgr bools; +} + +} // namespace diff --git a/src/lib/dhcpsrv/thread_resource_mgr.h b/src/lib/dhcpsrv/thread_resource_mgr.h new file mode 100644 index 0000000000..50e7f62899 --- /dev/null +++ b/src/lib/dhcpsrv/thread_resource_mgr.h @@ -0,0 +1,49 @@ +// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#ifndef THREAD_RESOURCE_MGR_H +#define THREAD_RESOURCE_MGR_H + +#include +#include +#include +#include + +namespace isc { +namespace dhcp { + +template +class ThreadResourceMgr { + typedef boost::shared_ptr ResourcePtr; +public: + /// @brief function to retrieve the specific resource of calling thread + /// This function returns the resource of the calling thread from the map + /// container or, in case it is not found, it creates a resource and adds it + /// to the map container + /// + /// @return the specific resource of the calling thread + ResourcePtr resource() { + std::lock_guard lock(&mutex_); + auto id = std::this_thread::get_id(); + if (map_.find(id) != map_.end()) { + return map_[id]; + } + ResourcePtr result(std::make_shared()); + map_[id] = result; + return result; + } +private: + /// @brief mutex used to keep the internal state consistent + std::mutex mutex_; + + /// @brief map container which holds the resources for each thread + std::unordered_map map_; +}; + +} // namespace dhcp +} // namespace isc + +#endif // THREAD_RESOURCE_MGR_H -- GitLab From 87cb74be94252605d2694ace8a51eb3ed9fe9e71 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Sep 2019 15:23:52 +0300 Subject: [PATCH 02/12] [#886, !508] added unit tests --- .../tests/thread_resource_mgr_unittest.cc | 239 +++++++++++++++++- src/lib/dhcpsrv/thread_resource_mgr.h | 4 +- 2 files changed, 231 insertions(+), 12 deletions(-) diff --git a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc index 5561567a3a..7c1227cd1e 100644 --- a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc @@ -10,54 +10,91 @@ #include +#include + +#include +#include #include +#include using namespace isc::dhcp; using namespace std; namespace { -/// @brief Test Fixture for testing isc::dhcp::ThreadResourceMgr -class ThreadResourceMgrTest : public ::testing::Test { -}; - +/// @brief test class to keep track of all constructed objects of a specific +/// class type +/// +/// @template parameter class to make this functionality available for a wide +/// range of 'similar' but distinct classes template -class Resource { +class Resource : public boost::noncopyable { public: - Resource() { + /// @brief Constructor + Resource() : data_() { lock_guard lk(mutex_); + // increase current number of instances of this class Resource::count_++; + // increase the total number of instances ever created Resource::created_count_++; + // check that this instance in new and should not be found in the + // verification set + EXPECT_TRUE(Resource::set_.find(&data_) == Resource::set_.end()); + // add this instance to the verification set + Resource::set_.emplace(&data_); } + /// @brief Destructor virtual ~Resource() { lock_guard lk(mutex_); + // decrease current number of instances of this class Resource::count_--; + // increase the total number of instances ever destroyed Resource::destroyed_count_++; + // check that this instance is found in the verification set + EXPECT_FALSE(Resource::set_.find(&data_) == Resource::set_.end()); + // remove this instance from the verification set + Resource::set_.erase(&data_); } + /// @brief count number of current allocated instances of the class + /// + /// @return number of current allocated instances of the class static uint32_t count() { lock_guard lk(mutex_); return Resource::count_; } + /// @brief count number of class instances ever created + /// + /// @return number of class instances ever created static uint32_t createdCount() { lock_guard lk(mutex_); return Resource::created_count_; } + /// @brief count number of class instances ever destroyed + /// + /// @return number of class instances ever destroyed static uint32_t destroyedCount() { lock_guard lk(mutex_); return Resource::destroyed_count_; } + /// @brief reset all statistics for this class static void reset() { lock_guard lk(mutex_); + // reset all statistics for this class Resource::count_ = 0; Resource::created_count_ = 0; Resource::destroyed_count_ = 0; + Resource::set_.clear(); } + private: + /// @brief data element + T data_; + /// @brief total number of instances at any given time static uint32_t count_; @@ -69,12 +106,194 @@ private: /// @brief mutex used to keep the internal state consistent static std::mutex mutex_; + + /// @brief set to fold + static std::set set_; }; -// This test verifies that each thread can access it's own allocated resource -TEST(ThreadResourceMgrTest, testThreadResources) { - ThreadResourceMgr integers; - ThreadResourceMgr bools; +template +uint32_t Resource::count_; +template +uint32_t Resource::created_count_; +template +uint32_t Resource::destroyed_count_; +template +std::mutex Resource::mutex_; +template +std::set Resource::set_; + +/// @brief Test Fixture for testing isc::dhcp::ThreadResourceMgr +class ThreadResourceMgrTest : public ::testing::Test { +public: + /// @brief Constructor + ThreadResourceMgrTest() : wait_(false) { + } + + /// @brief Destructor + ~ThreadResourceMgrTest() { + } + + /// @brief flag which indicates if working thread should wait for main + /// thread signal + /// + /// @return the wait flag + bool wait() { + return wait_; + } + + /// @brief function used by main thread to unblock processing threads + void signalThreads() { + lock_guard lk(wait_mutex_); + // clear the wait flag so that threads will no longer wait for the main + // thread signal + wait_ = false; + // wake all threads if waiting for main thread signal + wait_cv_.notify_all(); + } + + /// @brief reset resource manager for the template class and perform sanity + /// checks + template + void reset() { + get() = make_shared>>(); + sanityCheck(); + wait_ = true; + } + + /// @brief check statistics + /// + /// @param expected_count check equality of this value with the number of + /// class instances + /// @param expected_created check equality of this value with the number of + /// class instances ever created + /// @param expected_destroyed check equality of this value with the number + /// of class instances ever destroyed + template + void checkInstances(uint32_t expected_count, + uint32_t expected_created, + uint32_t expected_destroyed) { + ASSERT_EQ(Resource::count(), expected_count); + ASSERT_EQ(Resource::createdCount(), expected_created); + ASSERT_EQ(Resource::destroyedCount(), expected_destroyed); + } + + /// @brief sanity check that the number of created instances is equal to the + /// number of destroyed instances + template + void sanityCheck() { + ASSERT_EQ(Resource::createdCount(), Resource::destroyedCount()); + } + + /// @brief get the instance of the resource manager responsible for a + /// specific class + /// + /// @return the resource manager responsible for a specific class + template + shared_ptr>> &get() { + static shared_ptr>> container; + return container; + } + + /// @brief run function which accesses the resource allocated for the + /// calling thread and verifies the class statistics + /// @param expected_count check equality of this value with the number of + /// class instances + /// @param expected_created check equality of this value with the number of + /// class instances ever created + /// @param expected_destroyed check equality of this value with the number + /// of class instances ever destroyed + /// @param signal indicate if the function should wait for signal from main + /// thread or exit immediately + template + void run(uint32_t expected_count, + uint32_t expected_created, + uint32_t expected_destroyed, + bool signal = false) { + // get resource for this thread + auto left = get()->resource().get(); + // verify statistics + checkInstances(expected_count, expected_created, expected_destroyed); + // get the resource for this thread once more + auto right = get()->resource().get(); + // check that it is the same resource + ASSERT_EQ(left, right); + // verify statistics which should have not changed on multiple + // sequential requests for the same resource + checkInstances(expected_count, expected_created, expected_destroyed); + + if (signal) { + unique_lock lk(wait_mutex_); + // if specified, wait for signal from main thread + wait_cv_.wait(lk, [&]{ return (wait() == false); }); + } + } + +private: + /// @brief mutex used to keep the internal state consistent + /// related to the control of the main thread over the working threads exit + std::mutex wait_mutex_; + + /// @brief condition variable used to signal working threads to exit + condition_variable wait_cv_; + + /// @brief flag which indicates if working thread should wait for main + /// thread signal + bool wait_; +}; + +/// @brief This test verifies that each thread can access it's own allocated +/// resource. Multiple threads are created and run in parallel. The checks are +/// done while threads are still running. +/// It is very important for the threads to run in parallel and not just run and +/// join the thread as this will cause newer threads to use the old thread id +/// and receive the same resource. +/// If destroying threads, the resource manager should also be reset. +TEST_F(ThreadResourceMgrTest, testThreadResources) { + std::list> threads; + + // reset statistics for uint_32 type + reset(); + // call run function on main thread and verify statistics + run(1, 1, 0); + // call run on a different thread and verify statistics + threads.push_back(std::make_shared(std::bind( + &ThreadResourceMgrTest::run, this, 2, 2, 0, true))); + // call run again on a different thread and verify statistics + threads.push_back(std::make_shared(std::bind( + &ThreadResourceMgrTest::run, this, 3, 3, 0, true))); + // signal all threads + signalThreads(); + // wait for all threads to finish + for (auto &thread : threads) { + thread->join(); + } + // reset statistics for uint_32 type + reset(); + // verify statistics 0 instances, 3 created, 3 destroyed + checkInstances(0, 3, 3); + + threads.clear(); + + // reset statistics for bool type + reset(); + // call run function on main thread and verify statistics + run(1, 1, 0); + // call run on a different thread and verify statistics + threads.push_back(std::make_shared(std::bind( + &ThreadResourceMgrTest::run, this, 2, 2, 0, true))); + // call run again on a different thread and verify statistics + threads.push_back(std::make_shared(std::bind( + &ThreadResourceMgrTest::run, this, 3, 3, 0, true))); + // signal all threads + signalThreads(); + // wait for all threads to finish + for (auto &thread : threads) { + thread->join(); + } + // reset statistics for bool type + reset(); + // verify statistics 0 instances, 3 created, 3 destroyed + checkInstances(0, 3, 3); } } // namespace diff --git a/src/lib/dhcpsrv/thread_resource_mgr.h b/src/lib/dhcpsrv/thread_resource_mgr.h index 50e7f62899..405e92f611 100644 --- a/src/lib/dhcpsrv/thread_resource_mgr.h +++ b/src/lib/dhcpsrv/thread_resource_mgr.h @@ -17,7 +17,7 @@ namespace dhcp { template class ThreadResourceMgr { - typedef boost::shared_ptr ResourcePtr; + typedef std::shared_ptr ResourcePtr; public: /// @brief function to retrieve the specific resource of calling thread /// This function returns the resource of the calling thread from the map @@ -26,7 +26,7 @@ public: /// /// @return the specific resource of the calling thread ResourcePtr resource() { - std::lock_guard lock(&mutex_); + std::lock_guard lock(mutex_); auto id = std::this_thread::get_id(); if (map_.find(id) != map_.end()) { return map_[id]; -- GitLab From 7dee0b930c5a9a578577945d1b06c9194984372d Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Sep 2019 15:42:23 +0300 Subject: [PATCH 03/12] [#886, !508] clean up --- .../tests/thread_resource_mgr_unittest.cc | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc index 7c1227cd1e..42a67798d9 100644 --- a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc @@ -37,8 +37,7 @@ public: Resource::count_++; // increase the total number of instances ever created Resource::created_count_++; - // check that this instance in new and should not be found in the - // verification set + // check that this instance is not found in the verification set EXPECT_TRUE(Resource::set_.find(&data_) == Resource::set_.end()); // add this instance to the verification set Resource::set_.emplace(&data_); @@ -107,7 +106,7 @@ private: /// @brief mutex used to keep the internal state consistent static std::mutex mutex_; - /// @brief set to fold + /// @brief set to hold the distinct identification data of each instance static std::set set_; }; @@ -151,12 +150,16 @@ public: wait_cv_.notify_all(); } - /// @brief reset resource manager for the template class and perform sanity - /// checks + /// @brief reset resource manager for the specific class type and perform + /// sanity checks, then reset the wait flag so threads wait for the main + /// thread signal to exit template void reset() { + // reset the resource manager get() = make_shared>>(); + // perform sanity checks sanityCheck(); + // reset the wait flag wait_ = true; } @@ -177,17 +180,10 @@ public: ASSERT_EQ(Resource::destroyedCount(), expected_destroyed); } - /// @brief sanity check that the number of created instances is equal to the - /// number of destroyed instances - template - void sanityCheck() { - ASSERT_EQ(Resource::createdCount(), Resource::destroyedCount()); - } - /// @brief get the instance of the resource manager responsible for a - /// specific class + /// specific class type /// - /// @return the resource manager responsible for a specific class + /// @return the resource manager responsible for a specific class type template shared_ptr>> &get() { static shared_ptr>> container; @@ -229,6 +225,15 @@ public: } private: + /// @brief sanity check that the number of created instances is equal to the + /// number of destroyed instances + template + void sanityCheck() { + // the number of created instances should match the number of destroyed + // instances + ASSERT_EQ(Resource::createdCount(), Resource::destroyedCount()); + } + /// @brief mutex used to keep the internal state consistent /// related to the control of the main thread over the working threads exit std::mutex wait_mutex_; -- GitLab From 9135538df2310db20ff6a212c822bc072fe0f78f Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Thu, 12 Sep 2019 19:55:53 +0300 Subject: [PATCH 04/12] [#886, !508] fixed unit test --- .../tests/thread_resource_mgr_unittest.cc | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc index 42a67798d9..eef73cf2c0 100644 --- a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc @@ -125,21 +125,37 @@ std::set Resource::set_; class ThreadResourceMgrTest : public ::testing::Test { public: /// @brief Constructor - ThreadResourceMgrTest() : wait_(false) { + ThreadResourceMgrTest() : wait_thread_(false), wait_(false) { } /// @brief Destructor ~ThreadResourceMgrTest() { } + /// @brief flag which indicates if main thread should wait for the test + /// thread to start + /// + /// @return the wait flag + bool waitThread() { + return wait_thread_; + } + /// @brief flag which indicates if working thread should wait for main /// thread signal /// /// @return the wait flag - bool wait() { + bool waitMain() { return wait_; } + /// @brief block main thread until testing thread has processed the task + void wait() { + unique_lock lck(mutex_); + // wait for the testing thread to process + cv_.wait(lck, [&]{ return (waitThread() == false); }); + } + + /// @brief function used by main thread to unblock processing threads void signalThreads() { lock_guard lk(wait_mutex_); @@ -163,6 +179,11 @@ public: wait_ = true; } + /// @brief reset wait thread flag + void resetWaitThread() { + wait_thread_ = true; + } + /// @brief check statistics /// /// @param expected_count check equality of this value with the number of @@ -217,10 +238,19 @@ public: // sequential requests for the same resource checkInstances(expected_count, expected_created, expected_destroyed); + { + // make sure this thread has started + lock_guard lk(mutex_); + // reset wait thread flag + wait_thread_ = false; + // wake main thread if it is waiting for this thread to process + cv_.notify_all(); + } + if (signal) { unique_lock lk(wait_mutex_); // if specified, wait for signal from main thread - wait_cv_.wait(lk, [&]{ return (wait() == false); }); + wait_cv_.wait(lk, [&]{ return (waitMain() == false); }); } } @@ -234,6 +264,13 @@ private: ASSERT_EQ(Resource::createdCount(), Resource::destroyedCount()); } + /// @brief mutex used to keep the internal state consistent + std::mutex mutex_; + + /// @brief condition variable used to signal main thread that test thread + /// has started processing + condition_variable cv_; + /// @brief mutex used to keep the internal state consistent /// related to the control of the main thread over the working threads exit std::mutex wait_mutex_; @@ -241,6 +278,10 @@ private: /// @brief condition variable used to signal working threads to exit condition_variable wait_cv_; + /// @brief flag which indicates if main thread should wait for test thread + /// to start + bool wait_thread_; + /// @brief flag which indicates if working thread should wait for main /// thread signal bool wait_; @@ -260,12 +301,20 @@ TEST_F(ThreadResourceMgrTest, testThreadResources) { reset(); // call run function on main thread and verify statistics run(1, 1, 0); + // configure wait for test thread + resetWaitThread(); // call run on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( &ThreadResourceMgrTest::run, this, 2, 2, 0, true))); + // wait for the thread to process + wait(); + // configure wait for test thread + resetWaitThread(); // call run again on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( &ThreadResourceMgrTest::run, this, 3, 3, 0, true))); + // wait for the thread to process + wait(); // signal all threads signalThreads(); // wait for all threads to finish @@ -283,12 +332,20 @@ TEST_F(ThreadResourceMgrTest, testThreadResources) { reset(); // call run function on main thread and verify statistics run(1, 1, 0); + // configure wait for test thread + resetWaitThread(); // call run on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( &ThreadResourceMgrTest::run, this, 2, 2, 0, true))); + // wait for the thread to process + wait(); + // configure wait for test thread + resetWaitThread(); // call run again on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( &ThreadResourceMgrTest::run, this, 3, 3, 0, true))); + // wait for the thread to process + wait(); // signal all threads signalThreads(); // wait for all threads to finish -- GitLab From 9f2df497ff070530570c3a5ecf56a6c7a16ed243 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Mon, 4 Nov 2019 20:07:07 +0200 Subject: [PATCH 05/12] [#886, !508] renamed class and moved sources --- src/lib/dhcpsrv/Makefile.am | 1 - src/lib/dhcpsrv/tests/Makefile.am | 1 - src/lib/util/Makefile.am | 1 + src/lib/util/tests/Makefile.am | 1 + .../tests/thread_resource_unittest.cc} | 42 +++++++++---------- .../thread_resource.h} | 2 +- 6 files changed, 24 insertions(+), 24 deletions(-) rename src/lib/{dhcpsrv/tests/thread_resource_mgr_unittest.cc => util/tests/thread_resource_unittest.cc} (90%) rename src/lib/{dhcpsrv/thread_resource_mgr.h => util/thread_resource.h} (98%) diff --git a/src/lib/dhcpsrv/Makefile.am b/src/lib/dhcpsrv/Makefile.am index 1974c9a9b2..d0076959dd 100644 --- a/src/lib/dhcpsrv/Makefile.am +++ b/src/lib/dhcpsrv/Makefile.am @@ -118,7 +118,6 @@ libkea_dhcpsrv_la_SOURCES += lease_mgr_factory.cc lease_mgr_factory.h libkea_dhcpsrv_la_SOURCES += memfile_lease_mgr.cc memfile_lease_mgr.h libkea_dhcpsrv_la_SOURCES += memfile_lease_storage.h libkea_dhcpsrv_la_SOURCES += multi_threading_utils.h multi_threading_utils.cc -libkea_dhcpsrv_la_SOURCES += thread_resource_mgr.h if HAVE_MYSQL libkea_dhcpsrv_la_SOURCES += mysql_lease_mgr.cc mysql_lease_mgr.h diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index 8684f6379e..d9cac94776 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -124,7 +124,6 @@ libdhcpsrv_unittests_SOURCES += shared_networks_list_parser_unittest.cc libdhcpsrv_unittests_SOURCES += srv_config_unittest.cc libdhcpsrv_unittests_SOURCES += subnet_unittest.cc libdhcpsrv_unittests_SOURCES += test_get_callout_handle.cc test_get_callout_handle.h -libdhcpsrv_unittests_SOURCES += thread_resource_mgr_unittest.cc libdhcpsrv_unittests_SOURCES += triplet_unittest.cc libdhcpsrv_unittests_SOURCES += test_utils.cc test_utils.h libdhcpsrv_unittests_SOURCES += timer_mgr_unittest.cc diff --git a/src/lib/util/Makefile.am b/src/lib/util/Makefile.am index cb643556d9..0f0e529a17 100644 --- a/src/lib/util/Makefile.am +++ b/src/lib/util/Makefile.am @@ -28,6 +28,7 @@ libkea_util_la_SOURCES += state_model.cc state_model.h libkea_util_la_SOURCES += stopwatch.cc stopwatch.h libkea_util_la_SOURCES += stopwatch_impl.cc stopwatch_impl.h libkea_util_la_SOURCES += strutil.h strutil.cc +libkea_util_la_SOURCES += thread_resource.h libkea_util_la_SOURCES += time_utilities.h time_utilities.cc libkea_util_la_SOURCES += versioned_csv_file.h versioned_csv_file.cc libkea_util_la_SOURCES += watch_socket.cc watch_socket.h diff --git a/src/lib/util/tests/Makefile.am b/src/lib/util/tests/Makefile.am index 68b3e31ea3..ce86032a40 100644 --- a/src/lib/util/tests/Makefile.am +++ b/src/lib/util/tests/Makefile.am @@ -53,6 +53,7 @@ run_unittests_SOURCES += random_number_generator_unittest.cc run_unittests_SOURCES += staged_value_unittest.cc run_unittests_SOURCES += state_model_unittest.cc run_unittests_SOURCES += strutil_unittest.cc +run_unittests_SOURCES += thread_resource_unittest.cc run_unittests_SOURCES += time_utilities_unittest.cc run_unittests_SOURCES += range_utilities_unittest.cc run_unittests_SOURCES += signal_set_unittest.cc diff --git a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc b/src/lib/util/tests/thread_resource_unittest.cc similarity index 90% rename from src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc rename to src/lib/util/tests/thread_resource_unittest.cc index eef73cf2c0..09a35d8052 100644 --- a/src/lib/dhcpsrv/tests/thread_resource_mgr_unittest.cc +++ b/src/lib/util/tests/thread_resource_unittest.cc @@ -8,7 +8,7 @@ #include -#include +#include #include @@ -121,15 +121,15 @@ std::mutex Resource::mutex_; template std::set Resource::set_; -/// @brief Test Fixture for testing isc::dhcp::ThreadResourceMgr -class ThreadResourceMgrTest : public ::testing::Test { +/// @brief Test Fixture for testing isc::dhcp::ThreadResource +class ThreadResourceTest : public ::testing::Test { public: /// @brief Constructor - ThreadResourceMgrTest() : wait_thread_(false), wait_(false) { + ThreadResourceTest() : wait_thread_(false), wait_(false) { } /// @brief Destructor - ~ThreadResourceMgrTest() { + ~ThreadResourceTest() { } /// @brief flag which indicates if main thread should wait for the test @@ -166,13 +166,13 @@ public: wait_cv_.notify_all(); } - /// @brief reset resource manager for the specific class type and perform - /// sanity checks, then reset the wait flag so threads wait for the main - /// thread signal to exit + /// @brief reset resource for the specific class type and perform sanity + /// checks, then reset the wait flag so threads wait for the main thread + /// signal to exit template void reset() { - // reset the resource manager - get() = make_shared>>(); + // reset the resource + get() = make_shared>>(); // perform sanity checks sanityCheck(); // reset the wait flag @@ -201,13 +201,13 @@ public: ASSERT_EQ(Resource::destroyedCount(), expected_destroyed); } - /// @brief get the instance of the resource manager responsible for a - /// specific class type + /// @brief get the instance of the resource responsible for a specific class + /// type /// - /// @return the resource manager responsible for a specific class type + /// @return the resource responsible for a specific class type template - shared_ptr>> &get() { - static shared_ptr>> container; + shared_ptr>> &get() { + static shared_ptr>> container; return container; } @@ -293,8 +293,8 @@ private: /// It is very important for the threads to run in parallel and not just run and /// join the thread as this will cause newer threads to use the old thread id /// and receive the same resource. -/// If destroying threads, the resource manager should also be reset. -TEST_F(ThreadResourceMgrTest, testThreadResources) { +/// If destroying threads, the resource should also be reset. +TEST_F(ThreadResourceTest, testThreadResources) { std::list> threads; // reset statistics for uint_32 type @@ -305,14 +305,14 @@ TEST_F(ThreadResourceMgrTest, testThreadResources) { resetWaitThread(); // call run on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( - &ThreadResourceMgrTest::run, this, 2, 2, 0, true))); + &ThreadResourceTest::run, this, 2, 2, 0, true))); // wait for the thread to process wait(); // configure wait for test thread resetWaitThread(); // call run again on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( - &ThreadResourceMgrTest::run, this, 3, 3, 0, true))); + &ThreadResourceTest::run, this, 3, 3, 0, true))); // wait for the thread to process wait(); // signal all threads @@ -336,14 +336,14 @@ TEST_F(ThreadResourceMgrTest, testThreadResources) { resetWaitThread(); // call run on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( - &ThreadResourceMgrTest::run, this, 2, 2, 0, true))); + &ThreadResourceTest::run, this, 2, 2, 0, true))); // wait for the thread to process wait(); // configure wait for test thread resetWaitThread(); // call run again on a different thread and verify statistics threads.push_back(std::make_shared(std::bind( - &ThreadResourceMgrTest::run, this, 3, 3, 0, true))); + &ThreadResourceTest::run, this, 3, 3, 0, true))); // wait for the thread to process wait(); // signal all threads diff --git a/src/lib/dhcpsrv/thread_resource_mgr.h b/src/lib/util/thread_resource.h similarity index 98% rename from src/lib/dhcpsrv/thread_resource_mgr.h rename to src/lib/util/thread_resource.h index 405e92f611..bd360cbc7c 100644 --- a/src/lib/dhcpsrv/thread_resource_mgr.h +++ b/src/lib/util/thread_resource.h @@ -16,7 +16,7 @@ namespace isc { namespace dhcp { template -class ThreadResourceMgr { +class ThreadResource { typedef std::shared_ptr ResourcePtr; public: /// @brief function to retrieve the specific resource of calling thread -- GitLab From 1c8bd0942559ab19d79d2ed4f1da1b159ab6905c Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Mon, 4 Nov 2019 20:09:01 +0200 Subject: [PATCH 06/12] [#886, !508] removed spaces --- src/lib/util/tests/thread_resource_unittest.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lib/util/tests/thread_resource_unittest.cc b/src/lib/util/tests/thread_resource_unittest.cc index 09a35d8052..4977ced90e 100644 --- a/src/lib/util/tests/thread_resource_unittest.cc +++ b/src/lib/util/tests/thread_resource_unittest.cc @@ -155,7 +155,6 @@ public: cv_.wait(lck, [&]{ return (waitThread() == false); }); } - /// @brief function used by main thread to unblock processing threads void signalThreads() { lock_guard lk(wait_mutex_); -- GitLab From de10fab8200540e3785d69279cd476fb28df98c9 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Mon, 4 Nov 2019 22:28:19 +0200 Subject: [PATCH 07/12] [#886, !508] fixed header --- src/lib/util/thread_resource.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/util/thread_resource.h b/src/lib/util/thread_resource.h index bd360cbc7c..9a8e44a604 100644 --- a/src/lib/util/thread_resource.h +++ b/src/lib/util/thread_resource.h @@ -4,8 +4,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -#ifndef THREAD_RESOURCE_MGR_H -#define THREAD_RESOURCE_MGR_H +#ifndef THREAD_RESOURCE_H +#define THREAD_RESOURCE_H #include #include @@ -46,4 +46,4 @@ private: } // namespace dhcp } // namespace isc -#endif // THREAD_RESOURCE_MGR_H +#endif // THREAD_RESOURCE_H -- GitLab From 5d41ab703734823670ddca0cc19a5437cd3f5f80 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Mon, 28 Oct 2019 19:27:43 +0200 Subject: [PATCH 08/12] [#888,!573] implement pgsql thread handle --- src/lib/dhcpsrv/pgsql_host_data_source.cc | 59 +++++---- src/lib/dhcpsrv/pgsql_lease_mgr.cc | 67 +++++----- .../tests/pgsql_host_data_source_unittest.cc | 17 ++- src/lib/pgsql/pgsql_connection.cc | 121 +++++++++++++----- src/lib/pgsql/pgsql_connection.h | 91 ++++++------- src/lib/pgsql/pgsql_exchange.h | 3 +- .../pgsql/tests/pgsql_exchange_unittest.cc | 24 ++-- 7 files changed, 239 insertions(+), 143 deletions(-) diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc index 7ea6f61e6b..633ea483e7 100644 --- a/src/lib/dhcpsrv/pgsql_host_data_source.cc +++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc @@ -297,7 +297,7 @@ public: // most recently added host is different than the host id of the // currently processed host. if (hosts.empty() || row_host_id != hosts.back()->getHostId()) { - HostPtr host = retrieveHost(r, row, row_host_id); + HostPtr host(retrieveHost(r, row, row_host_id)); hosts.push_back(host); } } @@ -1263,7 +1263,7 @@ private: OptionPtr option_; }; -} // end of anonymous namespace +} // namespace namespace isc { namespace dhcp { @@ -1854,7 +1854,7 @@ TaggedStatementArray tagged_statements = { { // Using fixed scope_id = 3, which associates an option with host. {7, { OID_INT2, OID_BYTEA, OID_TEXT, - OID_VARCHAR, OID_BOOL, OID_TEXT, OID_INT8}, + OID_VARCHAR, OID_BOOL, OID_TEXT, OID_INT8 }, "insert_v4_host_option", "INSERT INTO dhcp4_options(code, value, formatted_value, space, " " persistent, user_context, host_id, scope_id) " @@ -1866,7 +1866,7 @@ TaggedStatementArray tagged_statements = { { // Using fixed scope_id = 3, which associates an option with host. {7, { OID_INT2, OID_BYTEA, OID_TEXT, - OID_VARCHAR, OID_BOOL, OID_TEXT, OID_INT8}, + OID_VARCHAR, OID_BOOL, OID_TEXT, OID_INT8 }, "insert_v6_host_option", "INSERT INTO dhcp6_options(code, value, formatted_value, space, " " persistent, user_context, host_id, scope_id) " @@ -1903,7 +1903,7 @@ TaggedStatementArray tagged_statements = { { } }; -}; // end anonymous namespace +} // namespace PgSqlHostDataSourceImpl:: PgSqlHostDataSourceImpl(const PgSqlConnection::ParameterMap& parameters) @@ -1927,7 +1927,7 @@ PgSqlHostDataSourceImpl(const PgSqlConnection::ParameterMap& parameters) isc_throw(DbOpenError, "PostgreSQL schema version mismatch: need version: " << code_version.first << "." << code_version.second - << " found version: " << db_version.first << "." + << " found version: " << db_version.first << "." << db_version.second); } @@ -1957,8 +1957,10 @@ uint64_t PgSqlHostDataSourceImpl::addStatement(StatementIndex stindex, PsqlBindArrayPtr& bind_array, const bool return_last_id) { + PgSqlHolder& holderHandle = conn_.handle(); uint64_t last_id = 0; - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, + + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, tagged_statements[stindex].nbparams, &bind_array->values_[0], &bind_array->lengths_[0], @@ -1987,7 +1989,9 @@ PgSqlHostDataSourceImpl::addStatement(StatementIndex stindex, bool PgSqlHostDataSourceImpl::delStatement(StatementIndex stindex, PsqlBindArrayPtr& bind_array) { - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, + PgSqlHolder& holderHandle = conn_.handle(); + + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, tagged_statements[stindex].nbparams, &bind_array->values_[0], &bind_array->lengths_[0], @@ -2062,9 +2066,10 @@ PgSqlHostDataSourceImpl:: getHostCollection(StatementIndex stindex, PsqlBindArrayPtr bind_array, boost::shared_ptr exchange, ConstHostCollection& result, bool single) const { + PgSqlHolder& holderHandle = conn_.handle(); exchange->clear(); - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, tagged_statements[stindex].nbparams, &bind_array->values_[0], &bind_array->lengths_[0], @@ -2110,29 +2115,34 @@ getHost(const SubnetID& subnet_id, // Return single record if present, else clear the host. ConstHostPtr result; - if (!collection.empty()) + if (!collection.empty()) { result = *collection.begin(); + } return (result); } -std::pair PgSqlHostDataSourceImpl::getVersion() const { +pair +PgSqlHostDataSourceImpl::getVersion() const { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_PGSQL_HOST_DB_GET_VERSION); + + PgSqlHolder& holderHandle = conn_.handle(); const char* version_sql = "SELECT version, minor FROM schema_version;"; - PgSqlResult r(PQexec(conn_, version_sql)); + + PgSqlResult r(PQexec(holderHandle, version_sql)); if(PQresultStatus(r) != PGRES_TUPLES_OK) { isc_throw(DbOperationError, "unable to execute PostgreSQL statement <" - << version_sql << ">, reason: " << PQerrorMessage(conn_)); + << version_sql << ">, reason: " << PQerrorMessage(holderHandle)); } - uint32_t version; - PgSqlExchange::getColumnValue(r, 0, 0, version); + uint32_t major; + PgSqlExchange::getColumnValue(r, 0, 0, major); uint32_t minor; PgSqlExchange::getColumnValue(r, 0, 1, minor); - return (std::make_pair(version, minor)); + return (make_pair(major, minor)); } void @@ -2455,8 +2465,8 @@ 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"); + isc_throw(BadValue, "PgSqlHostDataSource::get4(id, address): " + "wrong address type, address supplied is an IPv6 address"); } // Set up the WHERE clause value @@ -2475,8 +2485,9 @@ PgSqlHostDataSource::get4(const SubnetID& subnet_id, // Return single record if present, else clear the host. ConstHostPtr result; - if (!collection.empty()) + if (!collection.empty()) { result = *collection.begin(); + } return (result); } @@ -2550,7 +2561,8 @@ PgSqlHostDataSource::get6(const SubnetID& subnet_id, // Miscellaneous database methods. -std::string PgSqlHostDataSource::getName() const { +std::string +PgSqlHostDataSource::getName() const { std::string name = ""; try { name = impl_->conn_.getParameter("name"); @@ -2560,7 +2572,8 @@ std::string PgSqlHostDataSource::getName() const { return (name); } -std::string PgSqlHostDataSource::getDescription() const { +std::string +PgSqlHostDataSource::getDescription() const { return (std::string("Host data source that stores host information" "in PostgreSQL database")); } @@ -2583,5 +2596,5 @@ PgSqlHostDataSource::rollback() { impl_->conn_.rollback(); } -}; // end of isc::dhcp namespace -}; // end of isc namespace +} // namespace dhcp +} // namespace isc diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index 1ade373649..7180ea54fc 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -133,7 +133,7 @@ PgSqlTaggedStatement tagged_statements[] = { "SELECT address, hwaddr, client_id, " "valid_lifetime, extract(epoch from expire)::bigint, subnet_id, " "fqdn_fwd, fqdn_rev, hostname, " - "state, user_context " + "state, user_context " "FROM lease4 " "WHERE subnet_id = $1"}, @@ -307,6 +307,7 @@ PgSqlTaggedStatement tagged_statements[] = { "hwaddr = $13, hwtype = $14, hwaddr_source = $15, " "state = $16, user_context = $17 " "WHERE address = $18"}, + // ALL_LEASE4_STATS { 0, { OID_NONE }, "all_lease4_stats", @@ -333,7 +334,7 @@ PgSqlTaggedStatement tagged_statements[] = { { 0, { OID_NONE }, "all_lease6_stats", "SELECT subnet_id, lease_type, state, leases as state_count" - " FROM lease6_stat ORDER BY subnet_id, lease_type, state" }, + " FROM lease6_stat ORDER BY subnet_id, lease_type, state"}, // SUBNET_LEASE6_STATS { 1, { OID_INT8 }, @@ -341,7 +342,7 @@ PgSqlTaggedStatement tagged_statements[] = { "SELECT subnet_id, lease_type, state, leases as state_count" " FROM lease6_stat " " WHERE subnet_id = $1 " - " ORDER BY lease_type, state" }, + " ORDER BY lease_type, state"}, // SUBNET_RANGE_LEASE6_STATS { 2, { OID_INT8, OID_INT8 }, @@ -349,7 +350,8 @@ PgSqlTaggedStatement tagged_statements[] = { "SELECT subnet_id, lease_type, state, leases as state_count" " FROM lease6_stat " " WHERE subnet_id >= $1 and subnet_id <= $2 " - " ORDER BY subnet_id, lease_type, state" }, + " ORDER BY subnet_id, lease_type, state"}, + // End of list sentinel { 0, { 0 }, NULL, NULL} }; @@ -463,8 +465,7 @@ public: lease_ = lease; try { - addr_str_ = boost::lexical_cast - (lease->addr_.toUint32()); + addr_str_ = boost::lexical_cast(lease->addr_.toUint32()); bind_array.add(addr_str_); if (lease->hwaddr_ && !lease->hwaddr_->hwaddr_.empty()) { @@ -1018,10 +1019,11 @@ public: /// parameters (for all subnets), a subnet id for a single subnet, or /// a first and last subnet id for a subnet range. void start() { + PgSqlHolder& holderHandle = conn_.handle(); if (getSelectMode() == ALL_SUBNETS) { // Run the query with no where clause parameters. - result_set_.reset(new PgSqlResult(PQexecPrepared(conn_, statement_.name, + result_set_.reset(new PgSqlResult(PQexecPrepared(holderHandle, statement_.name, 0, 0, 0, 0, 0))); } else { // Set up the WHERE clause values @@ -1039,7 +1041,7 @@ public: } // Run the query with where clause parameters. - result_set_.reset(new PgSqlResult(PQexecPrepared(conn_, statement_.name, + result_set_.reset(new PgSqlResult(PQexecPrepared(holderHandle, statement_.name, parms.size(), &parms.values_[0], &parms.lengths_[0], &parms.formats_[0], 0))); } @@ -1132,7 +1134,7 @@ PgSqlLeaseMgr::PgSqlLeaseMgr(const DatabaseConnection::ParameterMap& parameters) // Now prepare the SQL statements. int i = 0; - for( ; tagged_statements[i].text != NULL ; ++i) { + for(; tagged_statements[i].text != NULL; ++i) { conn_.prepareStatement(tagged_statements[i]); } @@ -1158,7 +1160,9 @@ PgSqlLeaseMgr::getDBVersion() { bool PgSqlLeaseMgr::addLeaseCommon(StatementIndex stindex, PsqlBindArray& bind_array) { - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, + PgSqlHolder& holderHandle = conn_.handle(); + + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, tagged_statements[stindex].nbparams, &bind_array.values_[0], &bind_array.lengths_[0], @@ -1206,8 +1210,10 @@ void PgSqlLeaseMgr::getLeaseCollection(StatementIndex stindex, Exchange& exchange, LeaseCollection& result, bool single) const { + PgSqlHolder& holderHandle = conn_.handle(); const int n = tagged_statements[stindex].nbparams; - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, n, + + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, n, n > 0 ? &bind_array.values_[0] : NULL, n > 0 ? &bind_array.lengths_[0] : NULL, n > 0 ? &bind_array.formats_[0] : NULL, 0)); @@ -1273,8 +1279,7 @@ PgSqlLeaseMgr::getLease4(const isc::asiolink::IOAddress& addr) const { PsqlBindArray bind_array; // LEASE ADDRESS - std::string addr_str = boost::lexical_cast - (addr.toUint32()); + std::string addr_str = boost::lexical_cast(addr.toUint32()); bind_array.add(addr_str); // Get the data @@ -1709,7 +1714,9 @@ PgSqlLeaseMgr::updateLeaseCommon(StatementIndex stindex, LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_PGSQL_ADD_ADDR4).arg(tagged_statements[stindex].name); - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, + PgSqlHolder& holderHandle = conn_.handle(); + + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, tagged_statements[stindex].nbparams, &bind_array.values_[0], &bind_array.lengths_[0], @@ -1748,9 +1755,8 @@ PgSqlLeaseMgr::updateLease4(const Lease4Ptr& lease) { exchange4_->createBindForSend(lease, bind_array); // Set up the WHERE clause and append it to the SQL_BIND array - std::string addr4_ = boost::lexical_cast - (lease->addr_.toUint32()); - bind_array.add(addr4_); + std::string addr_str = boost::lexical_cast(lease->addr_.toUint32()); + bind_array.add(addr_str); // Drop to common update code updateLeaseCommon(stindex, bind_array, lease); @@ -1778,7 +1784,9 @@ PgSqlLeaseMgr::updateLease6(const Lease6Ptr& lease) { uint64_t PgSqlLeaseMgr::deleteLeaseCommon(StatementIndex stindex, PsqlBindArray& bind_array) { - PgSqlResult r(PQexecPrepared(conn_, tagged_statements[stindex].name, + PgSqlHolder& holderHandle = conn_.handle(); + + PgSqlResult r(PQexecPrepared(holderHandle, tagged_statements[stindex].name, tagged_statements[stindex].nbparams, &bind_array.values_[0], &bind_array.lengths_[0], @@ -1929,25 +1937,22 @@ PgSqlLeaseMgr::getVersion() const { LOG_DEBUG(dhcpsrv_logger, DHCPSRV_DBG_TRACE_DETAIL, DHCPSRV_PGSQL_GET_VERSION); + PgSqlHolder& holderHandle = conn_.handle(); const char* version_sql = "SELECT version, minor FROM schema_version;"; - PgSqlResult r(PQexec(conn_, version_sql)); + + PgSqlResult r(PQexec(holderHandle, version_sql)); if(PQresultStatus(r) != PGRES_TUPLES_OK) { isc_throw(DbOperationError, "unable to execute PostgreSQL statement <" - << version_sql << ", reason: " << PQerrorMessage(conn_)); + << version_sql << ">, reason: " << PQerrorMessage(holderHandle)); } - istringstream tmp; - uint32_t version; - tmp.str(PQgetvalue(r, 0, 0)); - tmp >> version; - tmp.str(""); - tmp.clear(); + uint32_t major; + PgSqlExchange::getColumnValue(r, 0, 0, major); uint32_t minor; - tmp.str(PQgetvalue(r, 0, 1)); - tmp >> minor; + PgSqlExchange::getColumnValue(r, 0, 1, minor); - return (make_pair(version, minor)); + return (make_pair(major, minor)); } void @@ -1960,5 +1965,5 @@ PgSqlLeaseMgr::rollback() { conn_.rollback(); } -}; // end of isc::dhcp namespace -}; // end of isc namespace +} // namespace dhcp +} // namespace isc diff --git a/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc index e8e149d482..88238f0d88 100644 --- a/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc @@ -119,9 +119,11 @@ public: PgSqlConnection conn(params); conn.openDatabase(); - PgSqlResult r(PQexec(conn, query.c_str())); + PgSqlHolder& holderHandle = conn.handle(); + + PgSqlResult r(PQexec(holderHandle, query.c_str())); if (PQresultStatus(r) != PGRES_TUPLES_OK) { - isc_throw(DbOperationError, "Query failed:" << PQerrorMessage(conn)); + isc_throw(DbOperationError, "Query failed:" << PQerrorMessage(holderHandle)); } int numrows = PQntuples(r); @@ -644,9 +646,14 @@ TEST_F(PgSqlHostDataSourceTest, testAddRollback) { PgSqlConnection conn(params); ASSERT_NO_THROW(conn.openDatabase()); - PgSqlResult r(PQexec(conn, "DROP TABLE IF EXISTS ipv6_reservations")); - ASSERT_TRUE (PQresultStatus(r) == PGRES_COMMAND_OK) - << " drop command failed :" << PQerrorMessage(conn); + PgSqlHolder& holderHandle = conn.handle(); + + ConstHostCollection collection = hdsptr_->getAll4(0); + ASSERT_EQ(collection.size(), 0); + + PgSqlResult r(PQexec(holderHandle, "DROP TABLE IF EXISTS ipv6_reservations")); + ASSERT_TRUE(PQresultStatus(r) == PGRES_COMMAND_OK) + << " drop command failed :" << PQerrorMessage(holderHandle); // Create a host with a reservation. HostPtr host = HostDataSourceUtils::initializeHost6("2001:db8:1::1", diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index af98f098cb..b33a7334aa 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -37,6 +37,62 @@ const int PGSQL_DEFAULT_CONNECTION_TIMEOUT = 5; // seconds const char PgSqlConnection::DUPLICATE_KEY[] = ERRCODE_UNIQUE_VIOLATION; +void +PgSqlHolder::setConnection(PGconn* connection) { + clearPrepared(); + if (pgconn_ != NULL) { + PQfinish(pgconn_); + } + pgconn_ = connection; + connected_ = false; + prepared_ = false; +} + +void +PgSqlHolder::clearPrepared() { + if (pgconn_ != NULL) { + // Deallocate the prepared queries. + if (PQstatus(pgconn_) == CONNECTION_OK) { + PgSqlResult r(PQexec(pgconn_, "DEALLOCATE all")); + if(PQresultStatus(r) != PGRES_COMMAND_OK) { + // Highly unlikely but we'll log it and go on. + DB_LOG_ERROR(PGSQL_DEALLOC_ERROR) + .arg(PQerrorMessage(pgconn_)); + } + } + } +} + +void +PgSqlHolder::openDatabase(PgSqlConnection& connection) { + if (connected_) { + return; + } + connected_ = true; + prepared_ = true; + connection.openDatabase(); + prepared_ = false; +} + +void +PgSqlHolder::prepareStatements(PgSqlConnection& connection) { + if (prepared_) { + return; + } + clearPrepared(); + // Prepare all statements queries with all known fields datatype + for (auto it = connection.statements_.begin(); + it != connection.statements_.end(); ++it) { + PgSqlResult r(PQprepare(pgconn_, (*it)->name, (*it)->text, + (*it)->nbparams, (*it)->types)); + if (PQresultStatus(r) != PGRES_COMMAND_OK) { + isc_throw(DbOperationError, "unable to prepare PostgreSQL statement: " + << (*it)->text << ", reason: " << PQerrorMessage(pgconn_)); + } + } + prepared_ = true; +} + PgSqlResult::PgSqlResult(PGresult *result) : result_(result), rows_(0), cols_(0) { if (!result) { @@ -103,7 +159,10 @@ PgSqlTransaction::PgSqlTransaction(PgSqlConnection& conn) PgSqlTransaction::~PgSqlTransaction() { // If commit() wasn't explicitly called, rollback. if (!committed_) { - conn_.rollback(); + try { + conn_.rollback(); + } catch (...) { + } } } @@ -114,28 +173,14 @@ PgSqlTransaction::commit() { } PgSqlConnection::~PgSqlConnection() { - if (conn_) { - // Deallocate the prepared queries. - if (PQstatus(conn_) == CONNECTION_OK) { - PgSqlResult r(PQexec(conn_, "DEALLOCATE all")); - if(PQresultStatus(r) != PGRES_COMMAND_OK) { - // Highly unlikely but we'll log it and go on. - DB_LOG_ERROR(PGSQL_DEALLOC_ERROR) - .arg(PQerrorMessage(conn_)); - } - } - } + statements_.clear(); + handle().clear(); } void PgSqlConnection::prepareStatement(const PgSqlTaggedStatement& statement) { - // Prepare all statements queries with all known fields datatype - PgSqlResult r(PQprepare(conn_, statement.name, statement.text, - statement.nbparams, statement.types)); - if(PQresultStatus(r) != PGRES_COMMAND_OK) { - isc_throw(DbOperationError, "unable to prepare PostgreSQL statement: " - << statement.text << ", reason: " << PQerrorMessage(conn_)); - } + statements_.push_back(&statement); + prepared_ = true; } void @@ -276,7 +321,10 @@ PgSqlConnection::openDatabase() { } // We have a valid connection, so let's save it to our holder - conn_.setConnection(new_conn); + PgSqlHolder& holderHandle = handle(); + holderHandle.setConnection(new_conn); + holderHandle.connected_ = true; + connected_ = true; } bool @@ -296,6 +344,9 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, // error class. Note, there is a severity field, but it can be // misleadingly returned as fatal. However, a loss of connectivity // can lead to a NULL sqlstate with a status of PGRES_FATAL_ERROR. + + PgSqlHolder& holderHandle = handle(); + const char* sqlstate = PQresultErrorField(r, PG_DIAG_SQLSTATE); if ((sqlstate == NULL) || ((memcmp(sqlstate, "08", 2) == 0) || // Connection Exception @@ -305,7 +356,7 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, (memcmp(sqlstate, "58", 2) == 0))) { // System error DB_LOG_ERROR(PGSQL_FATAL_ERROR) .arg(statement.name) - .arg(PQerrorMessage(conn_)) + .arg(PQerrorMessage(holderHandle)) .arg(sqlstate ? sqlstate : ""); // If there's no lost db callback or it returns false, @@ -321,7 +372,7 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, } // Apparently it wasn't fatal, so we throw with a helpful message. - const char* error_message = PQerrorMessage(conn_); + const char* error_message = PQerrorMessage(holderHandle); isc_throw(DbOperationError, "Statement exec failed:" << " for: " << statement.name << ", status: " << s << "sqlstate:[ " << (sqlstate ? sqlstate : "") @@ -332,9 +383,12 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, void PgSqlConnection::startTransaction() { DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, PGSQL_START_TRANSACTION); - PgSqlResult r(PQexec(conn_, "START TRANSACTION")); + + PgSqlHolder& holderHandle = handle(); + + PgSqlResult r(PQexec(holderHandle, "START TRANSACTION")); if (PQresultStatus(r) != PGRES_COMMAND_OK) { - const char* error_message = PQerrorMessage(conn_); + const char* error_message = PQerrorMessage(holderHandle); isc_throw(DbOperationError, "unable to start transaction" << error_message); } @@ -343,9 +397,12 @@ PgSqlConnection::startTransaction() { void PgSqlConnection::commit() { DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, PGSQL_COMMIT); - PgSqlResult r(PQexec(conn_, "COMMIT")); + + PgSqlHolder& holderHandle = handle(); + + PgSqlResult r(PQexec(holderHandle, "COMMIT")); if (PQresultStatus(r) != PGRES_COMMAND_OK) { - const char* error_message = PQerrorMessage(conn_); + const char* error_message = PQerrorMessage(holderHandle); isc_throw(DbOperationError, "commit failed: " << error_message); } } @@ -353,12 +410,16 @@ PgSqlConnection::commit() { void PgSqlConnection::rollback() { DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, PGSQL_ROLLBACK); - PgSqlResult r(PQexec(conn_, "ROLLBACK")); + + PgSqlHolder& holderHandle = handle(); + + PgSqlResult r(PQexec(holderHandle, "ROLLBACK")); if (PQresultStatus(r) != PGRES_COMMAND_OK) { - const char* error_message = PQerrorMessage(conn_); + const char* error_message = PQerrorMessage(holderHandle); isc_throw(DbOperationError, "rollback failed: " << error_message); } } -}; // end of isc::db namespace -}; // end of isc namespace +} // namespace db +} // namespace isc + diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index 339ec7eefc..40dd05df6f 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -7,6 +7,7 @@ #define PGSQL_CONNECTION_H #include +#include #include #include @@ -160,11 +161,13 @@ public: } private: - PGresult* result_; ///< Result set to be freed - int rows_; ///< Number of rows in the result set - int cols_; ///< Number of columns in the result set + PGresult* result_; ///< Result set to be freed + int rows_; ///< Number of rows in the result set + int cols_; ///< Number of columns in the result set }; +/// @brief Forward declaration to @ref PgSqlConnection. +class PgSqlConnection; /// @brief Postgresql connection handle Holder /// @@ -179,35 +182,34 @@ private: /// For this reason, the class is declared noncopyable. class PgSqlHolder : public boost::noncopyable { public: - /// @brief Constructor /// /// Sets the Postgresql API connector handle to NULL. /// - PgSqlHolder() : pgconn_(NULL) { + PgSqlHolder() : connected_(false), prepared_(false), pgconn_(NULL) { } /// @brief Destructor /// /// Frees up resources allocated by the connection. ~PgSqlHolder() { - if (pgconn_ != NULL) { - PQfinish(pgconn_); - } + clear(); } + void clear() { + setConnection(NULL); + } + + void clearPrepared(); + /// @brief Sets the connection to the value given /// /// @param connection - pointer to the Postgresql connection instance - void setConnection(PGconn* connection) { - if (pgconn_ != NULL) { - // Already set? Release the current connection first. - // Maybe this should be an error instead? - PQfinish(pgconn_); - } + void setConnection(PGconn* connection); - pgconn_ = connection; - } + void openDatabase(PgSqlConnection& connection); + + void prepareStatements(PgSqlConnection& connection); /// @brief Conversion Operator /// @@ -217,19 +219,13 @@ public: return (pgconn_); } - /// @brief Boolean Operator - /// - /// Allows testing the connection for emptiness: "if (holder)" - operator bool() const { - return (pgconn_); - } + bool connected_; ///< Flag to indicate openDatabase has been called private: - PGconn* pgconn_; ///< Postgresql connection -}; + bool prepared_; ///< Flag to indicate prepareStatements has been called -/// @brief Forward declaration to @ref PgSqlConnection. -class PgSqlConnection; + PGconn* pgconn_; ///< Postgresql connection +}; /// @brief RAII object representing a PostgreSQL transaction. /// @@ -304,8 +300,8 @@ public: /// @brief Constructor /// /// Initialize PgSqlConnection object with parameters needed for connection. - PgSqlConnection(const ParameterMap& parameters) - : DatabaseConnection(parameters) { + PgSqlConnection(const ParameterMap& parameters) : + DatabaseConnection(parameters), connected_(false), prepared_(false) { } /// @brief Destructor @@ -400,30 +396,37 @@ public: void checkStatementError(const PgSqlResult& r, PgSqlTaggedStatement& statement) const; - /// @brief PgSql connection handle + /// @brief Raw statements /// - /// This field is public, because it is used heavily from PgSqlLeaseMgr + /// This field is public, because it is used heavily from PgSqlConnection /// and from PgSqlHostDataSource. - PgSqlHolder conn_; + std::vector statements_; - /// @brief Conversion Operator + /// @brief PgSql connection handle /// - /// Allows the PgConnection object to be passed as the context argument to - /// PQxxxx functions. - operator PGconn*() const { - return (conn_); + /// This field is public, because it is used heavily from PgSqlLeaseMgr + /// and from PgSqlHostDataSource. + PgSqlHolder& handle() const { + auto result = handles_.resource(); + // thread_local std::shared_ptr result(std::make_shared()); + if (connected_) { + result->openDatabase(*(const_cast(this))); + } + if (prepared_) { + result->prepareStatements(*(const_cast(this))); + } + return *result; } - /// @brief Boolean Operator - /// - /// Allows testing the PgConnection for initialized connection - operator bool() const { - return (conn_); - } +private: + bool connected_; ///< Flag to indicate openDatabase has been called + + bool prepared_; ///< Flag to indicate prepareStatements has been called + mutable isc::dhcp::ThreadResourceMgr handles_; }; -}; // end of isc::db namespace -}; // end of isc namespace +} // namespace db +} // namespace isc #endif // PGSQL_CONNECTION_H diff --git a/src/lib/pgsql/pgsql_exchange.h b/src/lib/pgsql/pgsql_exchange.h index 4aa82b2235..fd9518a00f 100644 --- a/src/lib/pgsql/pgsql_exchange.h +++ b/src/lib/pgsql/pgsql_exchange.h @@ -70,7 +70,6 @@ struct PsqlBindArray { /// @return Returns true if there are no entries in the array, false /// otherwise. bool empty() const { - return (values_.empty()); } @@ -393,7 +392,7 @@ public: protected: /// @brief Stores text labels for columns, currently only used for /// logging and errors. - std::vectorcolumns_; + std::vector columns_; }; }; // end of isc::db namespace diff --git a/src/lib/pgsql/tests/pgsql_exchange_unittest.cc b/src/lib/pgsql/tests/pgsql_exchange_unittest.cc index 173665ac94..a1f597c3d7 100644 --- a/src/lib/pgsql/tests/pgsql_exchange_unittest.cc +++ b/src/lib/pgsql/tests/pgsql_exchange_unittest.cc @@ -198,18 +198,22 @@ public: " varchar_col VARCHAR(255) " "); "; - PgSqlResult r(PQexec(*conn_, sql)); + PgSqlHolder& holderHandle = conn_->handle(); + + PgSqlResult r(PQexec(holderHandle, sql)); ASSERT_EQ(PQresultStatus(r), PGRES_COMMAND_OK) - << " create basics table failed: " << PQerrorMessage(*conn_); + << " create basics table failed: " << PQerrorMessage(holderHandle); } /// @brief Destroys the basics table /// Asserts if the destruction fails void destroySchema() { if (conn_) { - PgSqlResult r(PQexec(*conn_, "DROP TABLE IF EXISTS basics;")); + PgSqlHolder& holderHandle = conn_->handle(); + + PgSqlResult r(PQexec(holderHandle, "DROP TABLE IF EXISTS basics;")); ASSERT_EQ(PQresultStatus(r), PGRES_COMMAND_OK) - << " drop basics table failed: " << PQerrorMessage(*conn_); + << " drop basics table failed: " << PQerrorMessage(holderHandle); } } @@ -227,10 +231,12 @@ public: /// Asserts if the result set status does not equal the expected outcome. void runSql(PgSqlResultPtr& r, const std::string& sql, int exp_outcome, int lineno) { - r.reset(new PgSqlResult(PQexec(*conn_, sql.c_str()))); + PgSqlHolder& holderHandle = conn_->handle(); + + r.reset(new PgSqlResult(PQexec(holderHandle, sql.c_str()))); ASSERT_EQ(PQresultStatus(*r), exp_outcome) << " runSql at line: " << lineno << " failed, sql:[" << sql - << "]\n reason: " << PQerrorMessage(*conn_); + << "]\n reason: " << PQerrorMessage(holderHandle); } /// @brief Executes a SQL statement and tests for an expected outcome @@ -250,7 +256,9 @@ public: PgSqlTaggedStatement& statement, PsqlBindArrayPtr bind_array, int exp_outcome, int lineno) { - r.reset(new PgSqlResult(PQexecPrepared(*conn_, statement.name, + PgSqlHolder& holderHandle = conn_->handle(); + + r.reset(new PgSqlResult(PQexecPrepared(holderHandle, statement.name, statement.nbparams, &bind_array->values_[0], &bind_array->lengths_[0], @@ -258,7 +266,7 @@ public: ASSERT_EQ(PQresultStatus(*r), exp_outcome) << " runPreparedStatement at line: " << lineno << " statement name:[" << statement.name - << "]\n reason: " << PQerrorMessage(*conn_); + << "]\n reason: " << PQerrorMessage(holderHandle); } /// @brief Fetches all of the rows currently in the table -- GitLab From 7da5946ceb92f32eba318f5e19691aad1bda5df2 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Tue, 29 Oct 2019 12:48:23 +0200 Subject: [PATCH 09/12] [#888,!573] updated doxygen --- src/lib/pgsql/pgsql_connection.cc | 34 +++++++++++++++++++++++-------- src/lib/pgsql/pgsql_connection.h | 34 ++++++++++++++++++++++++------- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index b33a7334aa..7ce1f7e537 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -39,25 +39,30 @@ const char PgSqlConnection::DUPLICATE_KEY[] = ERRCODE_UNIQUE_VIOLATION; void PgSqlHolder::setConnection(PGconn* connection) { + // clear prepared statements associated to current connection clearPrepared(); - if (pgconn_ != NULL) { - PQfinish(pgconn_); + // clear old database back-end object + if (pgsql_ != NULL) { + PQfinish(pgsql_); } - pgconn_ = connection; + // set new database back-end object + pgsql_ = connection; + // clear connected flag connected_ = false; + // clear prepared flag prepared_ = false; } void PgSqlHolder::clearPrepared() { - if (pgconn_ != NULL) { + if (pgsql_ != NULL) { // Deallocate the prepared queries. - if (PQstatus(pgconn_) == CONNECTION_OK) { - PgSqlResult r(PQexec(pgconn_, "DEALLOCATE all")); + if (PQstatus(pgsql_) == CONNECTION_OK) { + PgSqlResult r(PQexec(pgsql_, "DEALLOCATE all")); if(PQresultStatus(r) != PGRES_COMMAND_OK) { // Highly unlikely but we'll log it and go on. DB_LOG_ERROR(PGSQL_DEALLOC_ERROR) - .arg(PQerrorMessage(pgconn_)); + .arg(PQerrorMessage(pgsql_)); } } } @@ -65,31 +70,42 @@ PgSqlHolder::clearPrepared() { void PgSqlHolder::openDatabase(PgSqlConnection& connection) { + // return if holder has already called openDatabase if (connected_) { return; } + // set connected flag connected_ = true; + // set prepared flag to true so that PgSqlConnection::handle() within + // openDatabase function does not call prepareStatements before opening + // the new connection prepared_ = true; + // call openDatabase for this holder handle connection.openDatabase(); + // set prepared flag to false so that PgSqlConnection::handle() will + // call prepareStatements for this holder handle prepared_ = false; } void PgSqlHolder::prepareStatements(PgSqlConnection& connection) { + // return if holder has already called prepareStatemens if (prepared_) { return; } + // clear previous prepared statements clearPrepared(); // Prepare all statements queries with all known fields datatype for (auto it = connection.statements_.begin(); it != connection.statements_.end(); ++it) { - PgSqlResult r(PQprepare(pgconn_, (*it)->name, (*it)->text, + PgSqlResult r(PQprepare(pgsql_, (*it)->name, (*it)->text, (*it)->nbparams, (*it)->types)); if (PQresultStatus(r) != PGRES_COMMAND_OK) { isc_throw(DbOperationError, "unable to prepare PostgreSQL statement: " - << (*it)->text << ", reason: " << PQerrorMessage(pgconn_)); + << (*it)->text << ", reason: " << PQerrorMessage(pgsql_)); } } + // set prepared flag prepared_ = true; } diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index 40dd05df6f..bb60c93e57 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -184,31 +184,48 @@ class PgSqlHolder : public boost::noncopyable { public: /// @brief Constructor /// - /// Sets the Postgresql API connector handle to NULL. - /// - PgSqlHolder() : connected_(false), prepared_(false), pgconn_(NULL) { + /// Sets the PgSql API connector handle to NULL. + PgSqlHolder() : connected_(false), prepared_(false), pgsql_(NULL) { } /// @brief Destructor /// - /// Frees up resources allocated by the connection. + /// Frees up resources allocated by the connection holder. ~PgSqlHolder() { clear(); } + /// @brief Clear all resources + /// + /// Clear all resources. void clear() { setConnection(NULL); } + /// @brief Clear prepared statements + /// + /// Clear prepared statements. void clearPrepared(); /// @brief Sets the connection to the value given /// - /// @param connection - pointer to the Postgresql connection instance + /// Sets the database back-end object. + /// + /// @param connection - pointer to the PgSql connection instance void setConnection(PGconn* connection); + /// @brief Open database + /// + /// Open database and apply PgSql connection parameters. + /// + /// @param connection - associated connection which holds connection properties. void openDatabase(PgSqlConnection& connection); + /// @brief Prepare statements + /// + /// Prepare statements. + /// + /// @param connection - associated connection which holds the text statements. void prepareStatements(PgSqlConnection& connection); /// @brief Conversion Operator @@ -216,15 +233,18 @@ public: /// Allows the PgSqlHolder object to be passed as the context argument to /// PQxxxx functions. operator PGconn*() const { - return (pgconn_); + return (pgsql_); } + /// @brief The connected flag bool connected_; ///< Flag to indicate openDatabase has been called private: + /// @brief The prepared flag bool prepared_; ///< Flag to indicate prepareStatements has been called - PGconn* pgconn_; ///< Postgresql connection + /// @brief The PgSql database back-end object associated to this holder + PGconn* pgsql_; ///< Postgresql connection }; /// @brief RAII object representing a PostgreSQL transaction. -- GitLab From 8bab146721b3ed4ed7f8ad1cf70f08d390a87983 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Tue, 29 Oct 2019 13:12:59 +0200 Subject: [PATCH 10/12] [#888,!573] clean up code --- src/lib/pgsql/pgsql_connection.cc | 2 +- src/lib/pgsql/pgsql_connection.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index 7ce1f7e537..38318843f9 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -93,7 +93,7 @@ PgSqlHolder::prepareStatements(PgSqlConnection& connection) { if (prepared_) { return; } - // clear previous prepared statements + // clear previously prepared statements clearPrepared(); // Prepare all statements queries with all known fields datatype for (auto it = connection.statements_.begin(); diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index bb60c93e57..1ff452a206 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -428,7 +428,6 @@ public: /// and from PgSqlHostDataSource. PgSqlHolder& handle() const { auto result = handles_.resource(); - // thread_local std::shared_ptr result(std::make_shared()); if (connected_) { result->openDatabase(*(const_cast(this))); } -- GitLab From a100f77eaf9171e90b72d130095b72c628bea3c1 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Mon, 4 Nov 2019 22:26:38 +0200 Subject: [PATCH 11/12] [#888,!573] rebased --- src/lib/pgsql/pgsql_connection.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index 1ff452a206..54dd23cac1 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -7,7 +7,7 @@ #define PGSQL_CONNECTION_H #include -#include +#include #include #include @@ -442,7 +442,7 @@ private: bool prepared_; ///< Flag to indicate prepareStatements has been called - mutable isc::dhcp::ThreadResourceMgr handles_; + mutable isc::dhcp::ThreadResource handles_; }; } // namespace db -- GitLab From ba8060b97ea2edf1cc3dabc202a54f569c27b632 Mon Sep 17 00:00:00 2001 From: Razvan Becheriu Date: Wed, 6 Nov 2019 18:30:36 +0200 Subject: [PATCH 12/12] [#888,!573] use thread_local instead of ThreadResource --- src/lib/pgsql/pgsql_connection.cc | 12 ++++++++++++ src/lib/pgsql/pgsql_connection.h | 14 +------------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index 38318843f9..a2d7e6c56e 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -188,6 +188,18 @@ PgSqlTransaction::commit() { committed_ = true; } +PgSqlHolder& +PgSqlConnection::handle() const { + thread_local std::shared_ptr result(std::make_shared()); + if (connected_) { + result->openDatabase(*(const_cast(this))); + } + if (prepared_) { + result->prepareStatements(*(const_cast(this))); + } + return *result; +} + PgSqlConnection::~PgSqlConnection() { statements_.clear(); handle().clear(); diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index 54dd23cac1..c344f71d59 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -426,23 +426,11 @@ public: /// /// This field is public, because it is used heavily from PgSqlLeaseMgr /// and from PgSqlHostDataSource. - PgSqlHolder& handle() const { - auto result = handles_.resource(); - if (connected_) { - result->openDatabase(*(const_cast(this))); - } - if (prepared_) { - result->prepareStatements(*(const_cast(this))); - } - return *result; - } - + PgSqlHolder& handle() const; private: bool connected_; ///< Flag to indicate openDatabase has been called bool prepared_; ///< Flag to indicate prepareStatements has been called - - mutable isc::dhcp::ThreadResource handles_; }; } // namespace db -- GitLab