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.
---
