trafodion-codereview mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zellerh <...@git.apache.org>
Subject [GitHub] trafodion pull request #1429: [TRAFODION-2927] Refactor and improve UPDATE S...
Date Thu, 01 Feb 2018 01:45:29 GMT
Github user zellerh commented on a diff in the pull request:

    https://github.com/apache/trafodion/pull/1429#discussion_r165237344
  
    --- Diff: core/sql/ustat/hs_log.cpp ---
    @@ -389,44 +371,64 @@ NABoolean HSLogMan::GetLogFile(NAString & path, const char*
cqd_value)
     /*          until either StartLog() or         */
     /*          ClearLog() methods are called.     */
     /***********************************************/
    -void HSLogMan::StartLog(NABoolean needExplain, const char* logFileName)
    +void HSLogMan::StartLog(NABoolean needExplain)
       {
    -    // The GENERATE_EXPLAIN cqd captures explain data pertaining to dynamic
    -    // queries. Ordinarily we want it on, but for just-in-time logging triggered
    -    // by an error, we don't need it, and can't set it because HSFuncExecQuery
    -    // clears the diagnostics area, which causes the error to be lost.
    -    explainOn_ = needExplain;
    -    if (!needExplain ||
    -        HSFuncExecQuery("CONTROL QUERY DEFAULT GENERATE_EXPLAIN 'ON'") == 0)
    +    if (!logNeeded_)  // if logging isn't already on
           {
    -        CollIndex activeNodes = gpClusterInfo->numOfSMPs();
    -        if (logFileName)
    -        {
    -          *logFile_ = logFileName;
    -           currentTimingEvent_ = -1;
    -           startTime_[0] = 0;           /* reset timer           */
    -           logNeeded_ = TRUE;
    -        }
    -        else if(activeNodes > 2)
    -        {//we consider we are running on cluster 
    -         //if gpClusterInfo->numOfSMPs() > 2
    -           NABoolean ret = FALSE;
    -           if(GetLogFile(*logFile_, ActiveSchemaDB()->getDefaults().getValue(USTAT_LOG)))
    -             ret = ContainDirExist(logFile_->data());
    -
    -           if(ret)
    -             logNeeded_ = TRUE;
    -
    -           currentTimingEvent_ = -1;
    -           startTime_[0] = 0;           /* reset timer           */
    -        }
    +        // Construct logfile name incorporating process id and node number. Note that
    +        // the 2nd parameter of processhandle_decompose is named cpu but is actually
    +        // the node number for Seaquest (the 4th param, named nodenumber, is the cluster
    +        // number).
    +        Int32 nodeNum;
    +        Int32 pin;
    +        SB_Phandle_Type procHandle;
    +        XPROCESSHANDLE_GETMINE_(&procHandle);
    +        XPROCESSHANDLE_DECOMPOSE_(&procHandle, &nodeNum, &pin);
    +        long currentTime = (long)time(0);
    +
    +        const size_t MAX_FILENAME_SIZE = 50;
    +        char qualifiers[MAX_FILENAME_SIZE];
    +        sprintf(qualifiers, ".%d.%d.%ld.txt", nodeNum, pin, currentTime);
    +   
    +        std::string logFileName;
    +        QRLogger::getRootLogDirectory(CAT_SQL_USTAT, logFileName /* out */);
    +        if (logFileName.size() > 0)
    +          logFileName += '/';
    +
    +        const char * ustatLog = ActiveSchemaDB()->getDefaults().getValue(USTAT_LOG);
    +        const char * fileNameStem = ustatLog + strlen(ustatLog);
    +        if (ustatLog == fileNameStem) 
    +          fileNameStem = "ULOG";  // CQD USTAT_LOG is the empty string
             else
    -        {
    -          *logFile_ = ActiveSchemaDB()->getDefaults().getValue(USTAT_LOG);
    -           currentTimingEvent_ = -1;
    -           startTime_[0] = 0;           /* reset timer           */
    -           logNeeded_ = TRUE;
    -        }
    +          {
    +            // strip off any directory path name; we will always use the logs directory
    +            // as configured via QRLogger
    +            while ((fileNameStem > ustatLog) && (*(fileNameStem - 1) != '/'))
    +              fileNameStem--;
    +          }
    +
    +        logFileName += fileNameStem;
    +        logFileName += qualifiers;
    +
    +        NABoolean logStarted = QRLogger::startLogFile(CAT_SQL_USTAT,logFileName.c_str());
    --- End diff --
    
    This could allow a user to create a file anywhere the trafodion user has write access.
For UDFs, we are creating a sandbox in $TRAF_HOME/udr for different types of users, beginning
with a public area $TRAF_HOME/udr/public. I wonder whether it would be better here to sandbox
the log files as well, e.g. into something like $TRAF_HOME/logs/ustat_logs. In the UDF code
we also try to catch names like a/../../../export/lib/myfile that would escape the sandbox.
    
    A related comment/question (a non-issue), maybe I can bring it up here as well, as it's
related to the choice of log file directory: As far as I understand, these log files don't
conform to the format we are using for executor, TM, DCS, etc. Therefore they can't be read
by the event_log_reader UDF and usually they shouldn't be, unless they get placed into one
of the directories where this UDF looks (e.g. $TRAF_HOME/logs) and they have a name prefix
that matches the list supported by this UDF (e.g. "master_exec_" or "mon"). So, there should
be no issue. To be safe, I would recommend keeping the update stats log files in a separate
directory (maybe a subdirectory), so that the event_log_reader UDF doesn't have to look at
them each time it is called. Also, this avoids errors for all users if somebody accidentally
chooses one of the special prefixes like "mon" for their ustat log files.


---

Mime
View raw message