Relocated mutex so it no longer covers query parsing.

Author: jgates108

src/ccontrol/UserQueryFactory.cc

@@ -298,20 +298,19 @@ UserQuery::Ptr UserQueryFactory::newUserQuery(std::string const& aQuery, std::st
         }
         auto stmt = parser->getSelectStmt();
 
-        /// &&& not entirely sure what the _mutex was protecting _clientToQuery and _idToQuery aren't touched
-        /// &&& Not sure what internal elements of UserQueryFactory need protection?
-        /// &&&     None of its members look like they need protection.
-        /// &&& Is there an order of operations to things happening in the database?
-        std::unique_lock<std::mutex> uLock(_factoryMtx); //&&&
+        // Parsing doesn't need protection, but CSS in analyze query does and
+        // some of the uq related calls may require protection, but it
+        // isn't clear.
+        std::lock_guard<std::mutex> factoryLock(_factoryMtx);
 
         // handle special database/table names
         if (_stmtRefersToProcessListTable(stmt, defaultDb)) {
             return _makeUserQueryProcessList(stmt, _userQuerySharedResources, userQueryId, resultDb, aQuery,
-                    async);
+                                             async);
         }
         if (_stmtRefersQueriesTable(stmt, defaultDb)) {
             return _makeUserQueryQueries(stmt, _userQuerySharedResources, userQueryId, resultDb, aQuery,
-                    async);
+                                         async);
         }
 
         /// Determine if a SelectStmt is a simple COUNT(*) query and can be run as an optimized query.
@@ -343,7 +342,6 @@ UserQuery::Ptr UserQueryFactory::newUserQuery(std::string const& aQuery, std::st
         auto qs = std::make_shared<qproc::QuerySession>(_userQuerySharedResources->css,
                                                         _userQuerySharedResources->databaseModels, defaultDb,
                                                         _userQuerySharedResources->interactiveChunkLimit);
-        uLock.unlock(); // &&& unlock the mutex before going back to expensive operations.
         try {
             qs->analyzeQuery(query, stmt);
         } catch (...) {
@@ -385,12 +383,14 @@ UserQuery::Ptr UserQueryFactory::newUserQuery(std::string const& aQuery, std::st
         }
         return uq;
     } else if (UserQueryType::isSelectResult(query, userJobId)) {
+        std::lock_guard<std::mutex> factoryLock(_factoryMtx);
         auto uq = std::make_shared<UserQueryAsyncResult>(userJobId, _userQuerySharedResources->czarId,
                                                          _userQuerySharedResources->queryMetadata);
         LOGS(_log, LOG_LVL_DEBUG, "make UserQueryAsyncResult: userJobId=" << userJobId);
         return uq;
     } else if (UserQueryType::isShowProcessList(query, full)) {
         LOGS(_log, LOG_LVL_DEBUG, "make UserQueryProcessList: full=" << (full ? 'y' : 'n'));
+        std::lock_guard<std::mutex> factoryLock(_factoryMtx);
         try {
             return std::make_shared<UserQueryProcessList>(full, _userQuerySharedResources->qMetaSelect,
                                                           _userQuerySharedResources->czarId, userQueryId,
@@ -399,10 +399,12 @@ UserQuery::Ptr UserQueryFactory::newUserQuery(std::string const& aQuery, std::st
             return std::make_shared<UserQueryInvalid>(exc.what());
         }
     } else if (UserQueryType::isCall(query)) {
+        std::lock_guard<std::mutex> factoryLock(_factoryMtx);
         auto parser = std::make_shared<ParseRunner>(
                 query, _userQuerySharedResources->makeUserQueryResources(userQueryId, resultDb));
         return parser->getUserQuery();
     } else if (UserQueryType::isSet(query)) {
+        std::lock_guard<std::mutex> factoryLock(_factoryMtx);
         ParseRunner::Ptr parser;
         try {
             parser = std::make_shared<ParseRunner>(query);
@@ -421,6 +423,7 @@ UserQuery::Ptr UserQueryFactory::newUserQuery(std::string const& aQuery, std::st
         }
         return uq;
     } else {
+        std::lock_guard<std::mutex> factoryLock(_factoryMtx);
         // something that we don't recognize
         auto uq = std::make_shared<UserQueryInvalid>("Invalid or unsupported query: " + query);
         return uq;

src/ccontrol/UserQueryFactory.h

@@ -104,10 +104,11 @@ class UserQueryFactory : private boost::noncopyable {
     std::unique_ptr<boost::asio::io_service::work> _asioWork;
     std::unique_ptr<std::thread> _asioTimerThread;
 
-    std::mutex _factoryMtx;  // &&& is this needed
+    /// Protects CSS in `qs->analyzeQuery(query, stmt);` and
+    /// protects uq calls that alter the database.
+    std::mutex _factoryMtx;
 };
 
-
 }  // namespace lsst::qserv::ccontrol
 
 #endif  // LSST_QSERV_CCONTROL_USERQUERYFACTORY_H

src/czar/Czar.cc

@@ -224,12 +224,8 @@ SubmitResult Czar::submitQuery(string const& query, map<string, string> const& h
 
     // make new UserQuery
     // this is atomic
-    ccontrol::UserQuery::Ptr uq;
-    {
-        lock_guard<mutex> lock(_mutex);
-        uq = _uqFactory->newUserQuery(query, defaultDb, getQdispSharedResources(), userQueryId, msgTableName,
-                                      resultDb);
-    }
+    ccontrol::UserQuery::Ptr uq = _uqFactory->newUserQuery(query, defaultDb, getQdispSharedResources(),
+                                                           userQueryId, msgTableName, resultDb);
 
     // Add logging context with query ID
     QSERV_LOGCONTEXT_QUERY(uq->getQueryId());