MCOL-4920 Re-enables MDB to re-read Columnstore.xml if it was changed on disk (#2154)

Revert "Remove global lock from OAMCache"
This reverts commit 2aa5380d51.

dbcon/joblist/resourcemanager.cpp

@@ -385,16 +385,24 @@ bool  ResourceManager::getMysqldInfo(
 {
     static const std::string hostUserUnassignedValue("unassigned");
     // MCS will read username and pass from disk if the config changed.
-    bool reReadConfig = true;
+    u = getStringVal("CrossEngineSupport", "User", hostUserUnassignedValue);
-    u = getStringVal("CrossEngineSupport", "User", hostUserUnassignedValue, reReadConfig);
+    std::string encryptedPW = getStringVal("CrossEngineSupport", "Password", "");
-    std::string encryptedPW = getStringVal("CrossEngineSupport", "Password", "", reReadConfig);
     //This will return back the plaintext password if there is no MCSDATADIR/.secrets file present
     w = decrypt_password(encryptedPW);
     // MCS will not read username and pass from disk if the config changed.
     h = getStringVal("CrossEngineSupport", "Host", hostUserUnassignedValue);
     p = getUintVal("CrossEngineSupport", "Port", 0);
+    u = getStringVal("CrossEngineSupport", "User", "unassigned");
+    w = getStringVal("CrossEngineSupport", "Password", "");
 
-    return h != hostUserUnassignedValue && u != hostUserUnassignedValue && p;
+    bool rc = true;
+
+    if ((h.compare("unassigned") == 0) ||
+            (u.compare("unassigned") == 0) ||
+            (p == 0))
+        rc = false;
+
+    return rc;
 }
 
 bool ResourceManager::queryStatsEnabled() const

dbcon/joblist/resourcemanager.h

@@ -35,7 +35,6 @@
 #include "calpontselectexecutionplan.h"
 #include "resourcedistributor.h"
 #include "installdir.h"
-#include "branchpred.h"
 
 #include "atomicops.h"
 
@@ -561,10 +560,7 @@ private:
      * @param name the param name whose value is to be returned
      * @param defVal the default value returned if the value is missing
      */
-    std::string getStringVal(const std::string& section,
+    std::string getStringVal(const std::string& section, const std::string& name, const std::string& defVal) const;
-                             const std::string& name,
-                             const std::string& defVal,
-                             const bool reReadConfigIfNeeded = false) const;
 
     template<typename IntType>
     IntType getUintVal(const std::string& section, const std::string& name, IntType defval) const;
@@ -625,20 +621,13 @@ private:
 };
 
 
-inline std::string ResourceManager::getStringVal(const std::string& section,
+inline std::string ResourceManager::getStringVal(const std::string& section, const std::string& name, const std::string& defval) const
-                                                 const std::string& name,
-                                                 const std::string& defval,
-                                                 const bool reReadConfigIfNeeded) const
 {
-    std::string val = UNLIKELY(reReadConfigIfNeeded)
+    std::string val = fConfig->getConfig(section, name);
-                        ? fConfig->getFromActualConfig(section, name)
-                        : fConfig->getConfig(section, name);
 #ifdef DEBUGRM
     std::cout << "RM getStringVal for " << section << " : " << name << " val: " << val << " default: " << defval << std::endl;
 #endif
-    if (val.empty())
+    return (0 == val.length() ? defval : val);
-        val = defval;
-    return val;
 }
 
 template<typename IntType>

oam/oamcpp/oamcache.cpp

@@ -21,10 +21,10 @@
 #include <cassert>
 #include <map>
 #include <set>
-#include <atomic>
 using namespace std;
 
 #include <boost/thread.hpp>
+#include <boost/shared_ptr.hpp>
 using namespace boost;
 
 #define OAMCACHE_DLLEXPORT
@@ -38,27 +38,19 @@ using namespace boost;
 
 namespace
 {
-    oam::OamCache* oamCache = nullptr;
+oam::OamCache* oamCache = 0;
-    boost::mutex cacheLock;
+boost::mutex cacheLock;
 }
 
 namespace oam
 {
-std::atomic_bool hasOAMCache{false};
 
 OamCache* OamCache::makeOamCache()
 {
-    if (!hasOAMCache.load(std::memory_order_relaxed))
+    boost::mutex::scoped_lock lk(cacheLock);
-    {
-        boost::mutex::scoped_lock lk(cacheLock);
 
-        if (oamCache == nullptr)
+    if (oamCache == 0)
-        {
+        oamCache = new OamCache();
-            oamCache = new OamCache();
-            oamCache->checkReload();
-            hasOAMCache.store(true, std::memory_order_relaxed);
-        }
-    }
 
     return oamCache;
 }
@@ -88,7 +80,7 @@ void OamCache::checkReload()
 
     dbRootPMMap.reset(new map<int, int>());
 
-    //cerr << "reloading oamcache\n";
+    //cout << "reloading oamcache\n";
     for (uint32_t i = 0; i < dbroots.size(); i++)
     {
         oam.getDbrootPmConfig(dbroots[i], temp);
@@ -162,41 +154,64 @@ void OamCache::checkReload()
 
 OamCache::dbRootPMMap_t OamCache::getDBRootToPMMap()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return dbRootPMMap;
 }
 
 OamCache::dbRootPMMap_t OamCache::getDBRootToConnectionMap()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return dbRootConnectionMap;
 }
 
 OamCache::PMDbrootsMap_t OamCache::getPMToDbrootsMap()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return pmDbrootsMap;
 }
 
 uint32_t OamCache::getDBRootCount()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return numDBRoots;
 }
 
 DBRootConfigList& OamCache::getDBRootNums()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return dbroots;
 }
 
 std::vector<int>& OamCache::getModuleIds()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return moduleIds;
 }
 
 std::string OamCache::getOAMParentModuleName()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return OAMParentModuleName;
 }
 
 int OamCache::getLocalPMId()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
     // This comes from the file /var/lib/columnstore/local/module, not from the xml.
     // Thus, it's not refreshed during checkReload().
     if (mLocalPMId > 0)
@@ -237,11 +252,16 @@ int OamCache::getLocalPMId()
 
 string OamCache::getSystemName()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
+    checkReload();
     return systemName;
 }
 
 string OamCache::getModuleName()
 {
+    boost::mutex::scoped_lock lk(cacheLock);
+
     if (!moduleName.empty())
         return moduleName;
 

utils/configcpp/configcpp.cpp

@@ -51,7 +51,6 @@ namespace fs = boost::filesystem;
 #include <cstring>
 //#define NDEBUG
 #include <cassert>
-#include <atomic>
 
 #include "configcpp.h"
 
@@ -68,52 +67,51 @@ namespace fs = boost::filesystem;
 
 namespace
 {
-  static const std::string configDefaultFileName("Columnstore.xml");
+const fs::path defaultCalpontConfigFile("Columnstore.xml");
-  const fs::path defaultConfigFilePath(configDefaultFileName);
 }
 
 namespace config
 {
-  Config* globConfigInstancePtr = nullptr;
+Config::configMap_t Config::fInstanceMap;
-  Config::configMap_t Config::fInstanceMap;
+boost::mutex Config::fInstanceMapMutex;
-  boost::mutex Config::fInstanceMapMutex;
+boost::mutex Config::fXmlLock;
-  // duplicate to that in the Config class
+boost::mutex Config::fWriteXmlLock;
-  boost::mutex Config::fXmlLock;
-  // duplicate to that in the Config class
-  boost::mutex Config::fWriteXmlLock;
-  std::atomic_bool globHasConfig;
 
 Config* Config::makeConfig(const string& cf)
 {
-    if (cf.empty() || cf == configDefaultFileName)
+    return makeConfig(cf.c_str());
-    {
-        if (!globHasConfig.load(std::memory_order_relaxed))
-        {
-            // To save against the moment zero race when multiple threads hits
-            // this scope.
-            boost::mutex::scoped_lock lk(fInstanceMapMutex);
-            if (globConfigInstancePtr)
-                return globConfigInstancePtr;
-
-            // Make this configurable at least at compile-time.
-            std::string configFilePath = std::string(MCSSYSCONFDIR) + std::string("/columnstore/") + configDefaultFileName;
-            globConfigInstancePtr = new Config(configFilePath);
-            globHasConfig.store(true, std::memory_order_relaxed);
-        }
-        return globConfigInstancePtr;
-    }
-
-    boost::mutex::scoped_lock lk(fInstanceMapMutex);
-    if (fInstanceMap.find(cf) == fInstanceMap.end())
-    {
-        fInstanceMap[cf] = new Config(cf);
-    }
-    return fInstanceMap[cf];
 }
 
 Config* Config::makeConfig(const char* cf)
 {
-    return cf ? makeConfig(std::string(cf)) : makeConfig(std::string(""));
+    boost::mutex::scoped_lock lk(fInstanceMapMutex);
+
+    static string defaultFilePath;
+
+    if (cf == 0 || *cf == 0)
+    {
+        fs::path configFilePath;
+        configFilePath = fs::path(MCSSYSCONFDIR) / fs::path("columnstore") / defaultCalpontConfigFile;
+        defaultFilePath = configFilePath.string();
+
+        if (fInstanceMap.find(defaultFilePath) == fInstanceMap.end())
+        {
+            Config* instance = new Config(defaultFilePath);
+            fInstanceMap[defaultFilePath] = instance;
+        }
+
+        return fInstanceMap[defaultFilePath];
+    }
+
+    string configFile(cf);
+
+    if (fInstanceMap.find(configFile) == fInstanceMap.end())
+    {
+        Config* instance = new Config(configFile);
+        fInstanceMap[configFile] = instance;
+    }
+
+    return fInstanceMap[configFile];
 }
 
 Config::Config(const string& configFile) :
@@ -221,6 +219,8 @@ void Config::closeConfig(void)
 
 const string Config::getConfig(const string& section, const string& name)
 {
+    boost::recursive_mutex::scoped_lock lk(fLock);
+
     if (section.length() == 0 || name.length() == 0)
         throw invalid_argument("Config::getConfig: both section and name must have a length");
 
@@ -229,57 +229,46 @@ const string Config::getConfig(const string& section, const string& name)
         throw runtime_error("Config::getConfig: no XML document!");
     }
 
-    return fParser.getConfig(fDoc, section, name);
-}
-
-void Config::getConfig(const string& section, const string& name, vector<string>& values)
-{
-    if (section.length() == 0)
-        throw invalid_argument("Config::getConfig: section must have a length");
-
-    if (fDoc == 0)
-        throw runtime_error("Config::getConfig: no XML document!");
-
-    fParser.getConfig(fDoc, section, name, values);
-}
-
-const string Config::getFromActualConfig(const string& section, const string& name)
-{
-    if (section.length() == 0 || name.length() == 0)
-        throw invalid_argument("Config::getFromActualConfig: both section and name must have a length");
-
-    if (fDoc == 0)
-    {
-        throw runtime_error("Config::getFromActualConfig: no XML document!");
-    }
-
     struct stat statbuf;
 
     if (stat(fConfigFile.c_str(), &statbuf) == 0)
     {
-        // Config was changed on disk since last read.
         if (statbuf.st_mtime != fMtime)
         {
-            boost::recursive_mutex::scoped_lock lk(fLock);
+            closeConfig();
-            // To protect the potential race that happens right after
+            fMtime = statbuf.st_mtime;
-            // the config was changed.
+            parseDoc();
-            if (!stat(fConfigFile.c_str(), &statbuf) &&
-                statbuf.st_mtime != fMtime)
-            {
-                closeConfig();
-                fMtime = statbuf.st_mtime;
-                parseDoc();
-            }
         }
     }
 
     return fParser.getConfig(fDoc, section, name);
 }
 
+void Config::getConfig(const string& section, const string& name, vector<string>& values)
+{
+    boost::recursive_mutex::scoped_lock lk(fLock);
+
+    if (section.length() == 0)
+        throw invalid_argument("Config::getConfig: section must have a length");
+
+    if (fDoc == 0)
+        throw runtime_error("Config::getConfig: no XML document!");
+
+    struct stat statbuf;
+
+    if (stat(fConfigFile.c_str(), &statbuf) == 0)
+    {
+        if (statbuf.st_mtime != fMtime)
+        {
+            closeConfig();
+            fMtime = statbuf.st_mtime;
+            parseDoc();
+        }
+    }
+
+    fParser.getConfig(fDoc, section, name, values);
+}
 
-// NB The only utility that uses setConfig is setConfig binary.
-// !!!Don't ever ever use this in the engine code b/c it might result in a race
-// b/w getConfig and setConfig methods.!!!
 void Config::setConfig(const string& section, const string& name, const string& value)
 {
     boost::recursive_mutex::scoped_lock lk(fLock);
@@ -295,6 +284,7 @@ void Config::setConfig(const string& section, const string& name, const string&
     struct stat statbuf;
 
     memset(&statbuf, 0, sizeof(statbuf));
+
     if (stat(fConfigFile.c_str(), &statbuf) == 0)
     {
         if (statbuf.st_mtime != fMtime)
@@ -369,14 +359,14 @@ void Config::writeConfig(const string& configFile) const
     fs::rename(outFilePth, configFilePth);
 #else
 
-    const fs::path defaultConfigFilePathTemp("Columnstore.xml.temp");
+    const fs::path defaultCalpontConfigFileTemp("Columnstore.xml.temp");
     const fs::path saveCalpontConfigFileTemp("Columnstore.xml.columnstoreSave");
     const fs::path tmpCalpontConfigFileTemp("Columnstore.xml.temp1");
 
     fs::path etcdir = fs::path(MCSSYSCONFDIR) / fs::path("columnstore");
 
-    fs::path dcf = etcdir / fs::path(defaultConfigFilePath);
+    fs::path dcf = etcdir / fs::path(defaultCalpontConfigFile);
-    fs::path dcft = etcdir / fs::path(defaultConfigFilePathTemp);
+    fs::path dcft = etcdir / fs::path(defaultCalpontConfigFileTemp);
     fs::path scft = etcdir / fs::path(saveCalpontConfigFileTemp);
     fs::path tcft = etcdir / fs::path(tmpCalpontConfigFileTemp);
 
@@ -561,12 +551,6 @@ void Config::deleteInstanceMap()
     }
 
     fInstanceMap.clear();
-
-    if (globConfigInstancePtr)
-    {
-        delete globConfigInstancePtr;
-        globConfigInstancePtr = nullptr;
-    }
 }
 
 /* static */
@@ -619,6 +603,8 @@ int64_t Config::fromText(const std::string& text)
 
 time_t Config::getCurrentMTime()
 {
+    boost::recursive_mutex::scoped_lock lk(fLock);
+
     struct stat statbuf;
 
     if (stat(fConfigFile.c_str(), &statbuf) == 0)
@@ -627,7 +613,6 @@ time_t Config::getCurrentMTime()
         return 0;
 }
 
-// Utilized by getConfig only
 const vector<string> Config::enumConfig()
 {
     boost::recursive_mutex::scoped_lock lk(fLock);
@@ -652,7 +637,6 @@ const vector<string> Config::enumConfig()
     return fParser.enumConfig(fDoc);
 }
 
-// Utilized by getConfig only
 const vector<string> Config::enumSection(const string& section)
 {
     boost::recursive_mutex::scoped_lock lk(fLock);

utils/configcpp/configcpp.h

@@ -87,15 +87,6 @@ public:
     */
     EXPORT const std::string getConfig(const std::string& section, const std::string& name);
 
-    /** @brief get name's value from section
-    *
-    * get name's value from section in the current config file re-reading the
-    * config file if it was updated.
-    * @param section the name of the config file section to search
-    * @param name the param name whose value is to be returned
-    */
-    const std::string getFromActualConfig(const std::string& section, const std::string& name);
-
     /** @brief get all name's values from a section
     *
     * get name's values from section in the current config file.
@@ -113,8 +104,6 @@ public:
     * @param name the param name whose value is to be updated
     * @param value the param value
     */
-    // !!!Don't ever ever use this in the engine code b/c it might result in a race
-    // b/w getConfig and setConfig methods.!!!
     EXPORT void setConfig(const std::string& section, const std::string& name, const std::string& value);
 
     /** @brief delete name from section