RE: [Proposal] Adding Log File Capability to pg_createsubscriber

Dear Gayn,

Thanks for updating the patch. I resumed reviewing.

01.
```
+static void
+                       internal_log_file_write(const char *format,...) __attribute__((format(printf, 1, 2)));
```

pg_attribute_printf seems to be used in postgres.

02.
```
+static void
+make_dir(char *dir)
```

Since there are only two callers, I think no need to introduce the funciton.
E.g, make_outpudirs in pg_upgrade does mkdir() four times.

03.
```
#undef pg_log_debug
#define pg_log_debug(...) do{\
              if (internal_log_file_fp != NULL) \
                            internal_log_file_write(__VA_ARGS__); \
              else \
                            if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
                                          pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \
} while(0)
```

The patch ignores setting of log level if -l is specified. By default only
warnings/errors/fatals should be output, but even debug messages are output with
-l option. Checking logic is in pg_log_generic()->pg_log_generic_v() but we
cannot reach if internal_log_file_fp is set.

Also, are there any examples to undefine these macros? It's bit surprising for
me; I prefer some inline functions instead.


04.
```
+                       case 'l':
+                               opt.log_dir = pg_strdup(optarg);
+                               canonicalize_path(opt.log_dir);
+                               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 feel creating the log file is too early, otherwise the issue Shveta raised the
at #2 could happen [1].
I feel it's OK putting after the if (opt.pub_conninfo_str == NULL). Because
there is the first place where call pg_log_info, after doing very simple validation.

05.
Let me confirm one point. IIUC, freopen() can be used to replace the stderr to
the file, and this may able to remove all #undef macros. The downside of this
approach is that even ERROR/FATAL messages would be on the file and nothing
appears on the terminal. See DebugFileOpen() the example.

Do you think that we may not use freopen() as-is? Do others have variants?

06.
I added this thread and the cfbot cannot accept your patch [2]. Please fix.

[1]: https://www.postgresql.org/message-id/CAJpy0uBPvz6S9VE8sLYmoju4BGYh94uks%2BUTocPdD094xqmZ2w%40mail.gmail.com
[2]: https://commitfest.postgresql.org/patch/6592/

Best regards,
Hayato Kuroda
FUJITSU LIMITED