Merge pull request #1123 from pmenon/fix-memory-leak

Fix memory leak in query history logging

Author: linmagit

src/brain/query_logger.cpp

@@ -11,22 +11,40 @@
 //===----------------------------------------------------------------------===//
 
 #include "brain/query_logger.h"
+
 #include "catalog/query_history_catalog.h"
 #include "concurrency/transaction_context.h"
 #include "concurrency/transaction_manager_factory.h"
-#include "parser/pg_query.h"
 
 namespace peloton {
 namespace brain {
 
+QueryLogger::Fingerprint::Fingerprint(const std::string &query)
+    : query_(query),
+      fingerprint_(""),
+      fingerprint_result_(pg_query_fingerprint(query.c_str())) {
+  if (fingerprint_result_.hexdigest != nullptr) {
+    fingerprint_ = fingerprint_result_.hexdigest;
+  }
+}
+
+QueryLogger::Fingerprint::~Fingerprint() {
+  pg_query_free_fingerprint_result(fingerprint_result_);
+}
+
 void QueryLogger::LogQuery(std::string query_string, uint64_t timestamp) {
   auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
-  auto txn = txn_manager.BeginTransaction();
-  std::string fingerprint = pg_query_fingerprint(query_string.c_str()).hexdigest;
+  auto *txn = txn_manager.BeginTransaction();
+
+  // Perform fingerprint
+  Fingerprint fingerprint{query_string};
 
-  catalog::QueryHistoryCatalog::GetInstance()->InsertQueryHistory(
-      query_string, fingerprint, timestamp, nullptr, txn);
+  // Log query + fingerprint
+  auto &query_history_catalog = catalog::QueryHistoryCatalog::GetInstance();
+  query_history_catalog.InsertQueryHistory(
+      query_string, fingerprint.GetFingerprint(), timestamp, nullptr, txn);
 
+  // We're done
   txn_manager.CommitTransaction(txn);
 }
 

src/catalog/query_history_catalog.cpp

@@ -6,25 +6,23 @@
 //
 // Identification: src/catalog/query_history_catalog.cpp
 //
-// Copyright (c) 2015-18, Carnegie Mellon University Database Group
+// Copyright (c) 2015-2018, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
 #include "catalog/query_history_catalog.h"
 
 #include "catalog/catalog.h"
-#include "executor/logical_tile.h"
-#include "parser/pg_query.h"
 #include "storage/data_table.h"
 #include "type/value_factory.h"
 
 namespace peloton {
 namespace catalog {
 
-QueryHistoryCatalog *QueryHistoryCatalog::GetInstance(
+QueryHistoryCatalog &QueryHistoryCatalog::GetInstance(
     concurrency::TransactionContext *txn) {
   static QueryHistoryCatalog query_history_catalog{txn};
-  return &query_history_catalog;
+  return query_history_catalog;
 }
 
 QueryHistoryCatalog::QueryHistoryCatalog(concurrency::TransactionContext *txn)
@@ -36,22 +34,23 @@ QueryHistoryCatalog::QueryHistoryCatalog(concurrency::TransactionContext *txn)
                       "timestamp      TIMESTAMP NOT NULL);",
                       txn) {}
 
-QueryHistoryCatalog::~QueryHistoryCatalog() {}
+QueryHistoryCatalog::~QueryHistoryCatalog() = default;
 
-bool QueryHistoryCatalog::InsertQueryHistory(const std::string &query_string, 
-                                  std::string &fingerprint, uint64_t timestamp,
-                                  type::AbstractPool *pool,
-                                  concurrency::TransactionContext *txn) {
+bool QueryHistoryCatalog::InsertQueryHistory(
+    const std::string &query_string, const std::string &fingerprint,
+    uint64_t timestamp, type::AbstractPool *pool,
+    concurrency::TransactionContext *txn) {
   std::unique_ptr<storage::Tuple> tuple(
       new storage::Tuple(catalog_table_->GetSchema(), true));
 
-  auto val0 = type::ValueFactory::GetVarcharValue(query_string, pool);
-  auto val1 = type::ValueFactory::GetVarcharValue(fingerprint, pool);
+  auto val0 = type::ValueFactory::GetVarcharValue(query_string);
+  auto val1 = type::ValueFactory::GetVarcharValue(fingerprint);
   auto val2 = type::ValueFactory::GetTimestampValue(timestamp);
 
-  tuple->SetValue(ColumnId::QUERY_STRING, val0, pool);
-  tuple->SetValue(ColumnId::FINGERPRINT, val1, pool);
-  tuple->SetValue(ColumnId::TIMESTAMP, val2, pool);
+  tuple->SetValue(ColumnId::QUERY_STRING, val0,
+                  pool != nullptr ? pool : &pool_);
+  tuple->SetValue(ColumnId::FINGERPRINT, val1, pool != nullptr ? pool : &pool_);
+  tuple->SetValue(ColumnId::TIMESTAMP, val2, pool != nullptr ? pool : &pool_);
 
   // Insert the tuple
   return InsertTuple(std::move(tuple), txn);

src/include/brain/query_logger.h

@@ -12,8 +12,11 @@
 
 #pragma once
 
+#include <cstdint>
 #include <string>
 
+#include "parser/pg_query.h"
+
 namespace peloton {
 namespace brain {
 
@@ -25,8 +28,28 @@ class QueryLogger {
  public:
   QueryLogger() = default;
 
-  /*
+  class Fingerprint {
+   public:
+    /// Constructor
+    explicit Fingerprint(const std::string &query);
+
+    /// Destructor
+    ~Fingerprint();
+
+    /// Get original string
+    const std::string &GetQueryString() { return query_; }
+    const std::string &GetFingerprint() { return fingerprint_; }
+
+   private:
+    // Accessors
+    std::string query_;
+    std::string fingerprint_;
+    PgQueryFingerprintResult fingerprint_result_;
+  };
+
+  /**
    * @brief This function logs the query into query_history_catalog
+   *
    * @param the sql string corresponding to the query
    * @param timestamp of the transaction that executed the query
    */

src/include/catalog/query_history_catalog.h

@@ -6,7 +6,7 @@
 //
 // Identification: src/include/catalog/query_history_catalog.h
 //
-// Copyright (c) 2015-18, Carnegie Mellon University Database Group
+// Copyright (c) 2015-2018, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
@@ -23,6 +23,7 @@
 #pragma once
 
 #include "catalog/abstract_catalog.h"
+#include "type/ephemeral_pool.h"
 
 #define QUERY_HISTORY_CATALOG_NAME "pg_query_history"
 
@@ -34,14 +35,14 @@ class QueryHistoryCatalog : public AbstractCatalog {
   ~QueryHistoryCatalog();
 
   // Global Singleton
-  static QueryHistoryCatalog *GetInstance(
+  static QueryHistoryCatalog &GetInstance(
       concurrency::TransactionContext *txn = nullptr);
 
   //===--------------------------------------------------------------------===//
   // write Related API
   //===--------------------------------------------------------------------===//
   bool InsertQueryHistory(const std::string &query_string,
-                          std::string &fingerprint, uint64_t timestamp,
+                          const std::string &fingerprint, uint64_t timestamp,
                           type::AbstractPool *pool,
                           concurrency::TransactionContext *txn);
 
@@ -54,6 +55,8 @@ class QueryHistoryCatalog : public AbstractCatalog {
  private:
   QueryHistoryCatalog(concurrency::TransactionContext *txn);
 
+  // Pool to use for variable length strings
+  type::EphemeralPool pool_;
 };
 
 }  // namespace catalog

test/brain/query_logger_test.cpp

@@ -6,43 +6,45 @@
 //
 // Identification: test/brain/query_logger_test.cpp
 //
-// Copyright (c) 2015-16, Carnegie Mellon University Database Group
+// Copyright (c) 2015-2018, Carnegie Mellon University Database Group
 //
 //===----------------------------------------------------------------------===//
 
 #include "common/harness.h"
+
+#include "brain/query_logger.h"
 #include "sql/testing_sql_util.h"
 #include "settings/settings_manager.h"
-#include "parser/pg_query.h"
-
-using std::vector;
-using std::string;
 
 namespace peloton {
 namespace test {
 
 class QueryLoggerTests : public PelotonTest {
  protected:
-  virtual void SetUp() override {
+  void SetUp() override {
     settings::SettingsManager::SetBool(settings::SettingId::brain, true);
     PelotonInit::Initialize();
 
     // query to check that logging is done
     select_query_ =
         "SELECT query_string, fingerprint FROM pg_catalog.pg_query_history;";
-    select_query_fingerprint_ =
-        pg_query_fingerprint(select_query_.c_str()).hexdigest;
+
+    brain::QueryLogger::Fingerprint fingerprint{select_query_};
+    select_query_fingerprint_ = fingerprint.GetFingerprint();
+
     wait_time_ = 2;
   }
 
-  virtual void TearDown() override { PelotonInit::Shutdown(); }
+  void TearDown() override { PelotonInit::Shutdown(); }
 
   // Executes the given query and then checks if the queries that are executed
   // till now are actually logged
-  void TestSimpleUtil(string const &test_query,
-                      vector<std::string> &expected_result) {
-    string test_query_fingerprint =
-        pg_query_fingerprint(test_query.c_str()).hexdigest;
+  void TestSimpleUtil(std::string const &test_query,
+                      std::vector<std::string> &expected_result) {
+    brain::QueryLogger::Fingerprint fingerprint{test_query};
+
+    std::string test_query_fingerprint = fingerprint.GetFingerprint();
+
     expected_result.push_back(test_query + "|" + test_query_fingerprint);
     TestingSQLUtil::ExecuteSQLQuery(test_query.c_str());
 
@@ -57,14 +59,16 @@ class QueryLoggerTests : public PelotonTest {
   }
 
   // Executes the given query and then checks if the queries that are executed
-  // till now are actually logged only when the transaction commits. Otherwise
+  // until now are actually logged only when the transaction commits. Otherwise
   // stores to queries for checking this later when commit happens.
-  void TestTransactionUtil(string const &test_query,
-                           vector<std::string> &expected_result,
+  void TestTransactionUtil(std::string const &test_query,
+                           std::vector<std::string> &expected_result,
                            bool committed) {
-    static vector<std::string> temporary_expected_result;
-    string test_query_fingerprint =
-        pg_query_fingerprint(test_query.c_str()).hexdigest;
+    static std::vector<std::string> temporary_expected_result;
+
+    brain::QueryLogger::Fingerprint fingerprint{test_query};
+    std::string test_query_fingerprint = fingerprint.GetFingerprint();
+
     temporary_expected_result.push_back(test_query + "|" +
                                         test_query_fingerprint);
     TestingSQLUtil::ExecuteSQLQuery(test_query.c_str());
@@ -100,15 +104,20 @@ class QueryLoggerTests : public PelotonTest {
   }
 
  protected:
-  string select_query_;  // fixed query to check the queries logged in the table
-  string select_query_fingerprint_;  // fingerprint for the fixed query
-  int wait_time_;  // time to wait in seconds for the query to log into the
-                   // table
+  // fixed query to check the queries logged in the table
+  std::string select_query_;
+
+  // fingerprint for the fixed query
+  std::string select_query_fingerprint_;
+
+  // time to wait in seconds for the query to log into the table
+  int wait_time_;
 };
 
 // Testing the functionality of query logging
 TEST_F(QueryLoggerTests, QueriesTest) {
-  vector<std::string> expected_result;  // used to store the expected result
+  // used to store the expected result
+  std::vector<std::string> expected_result;
 
   // create the table and do some inserts and check
   TestSimpleUtil("CREATE TABLE test(a INT);", expected_result);