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