On Wed, Mar 18, 2026 at 11:43 AM Kuroda, Hayato/黒田 隼人 <[email protected]> wrote: > > 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?
+1, otherwise it will simply create a directory and error out after that if the command is wrong. We can avoid creating a directory. Patch has a compilation issue, logfile_open() needs a second argument in main(). If we fix that, it gives a segmentation fault in make_output_dirs() as logdir is not allocated, earlier it was an array, now a NULL pointer. Kuroda-san is going to post a new patch soon. thanks Shveta
