RE: [Proposal] Adding Log File Capability to pg_createsubscriber

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