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
