Dear Gyan, Thanks for updating. Not sure you have addressed my comments as well, but I reviewed again.
01.
```
+#undef pg_log_warning
+#define pg_log_warning(...) \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
+ pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__)
```
IIUC it's more common that to have do {...} while if the marco function has
several lines.
02.
```
+#undef pg_log_info
+#define pg_log_info(...) do { \
+ if (internal_log_file_fp != NULL) \
+ internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
+ else \
+ pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
+} while(0)
```
Missing update?
03.
I think pg_log_error faimily should be also output on the file, but it misses.
04.
```
+static void
+make_dir(const char *dir)
```
I think this is not needed, we can follow the approach done on the pg_upgrade.
The message "directory %s created" may be removed, but I think it's ok.
05.
```
+ /* Build timestamp directory path */
+ len = snprintf(timestamp_dir, MAXPGPATH, "%s/%s", log_dir, timestamp);
+
+ if (len >= MAXPGPATH)
+ pg_fatal("directory path for log files, %s/%s, is too long",
+ log_dir, timestamp);
```
These checks should be done before creating the base directory.
06.
```
+static char *log_timestamp = NULL; /* Timestamp to be used in all log file
+ *
names */
```
I feel it's more efficient to directly have the directory where log exists.
07.
```
+ if (opt.log_dir != NULL)
+ {
+ char *internal_log_file;
+
+ make_output_dirs(opt.log_dir);
+ internal_log_file = psprintf("%s/%s/%s.log", opt.log_dir,
log_timestamp,
+
INTERNAL_LOG_FILE_NAME);
+
+ if ((internal_log_file_fp = logfile_open(internal_log_file,
"a")) == NULL)
+ pg_fatal("could not open log file \"%s\": %m",
internal_log_file);
+ }
```
I still think it can be put bit later; after validating simple messages.
How about others?
08.
```
+static void
+internal_log_file_write(enum pg_log_level level, const char *format,...)
+{
+ va_list args;
+
+ if (level < __pg_log_level)
+ return;
+
+ if (internal_log_file_fp == NULL)
+ return;
+
+ va_start(args, format);
+ vfprintf(internal_log_file_fp, format, args);
+ va_end(args);
+
+ fprintf(internal_log_file_fp, "\n");
+ fflush(internal_log_file_fp);
+}
```
internal_log_file_fp has already been checked before calling, so it can be
Assert().
Also, the translated string should be passed to vfprintf().
My small tests shown that changing from "format" to "_(format)" is enough, but
not sure
other platforms. Some tests are needed.
09.
```
+ if (opt->log_dir != NULL)
+ out_file = psprintf("%s/%s/%s.log", opt->log_dir,
log_timestamp, SERVER_LOG_FILE_NAME);
+ else
+ out_file = DEVNULL;
```
I still think comments should be atop here.
Attached patch includes above changes.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
kuroda_diffs_atop_v10.diffs
Description: kuroda_diffs_atop_v10.diffs
