Re: [Proposal] Adding Log File Capability to pg_createsubscriber

On Wed, Mar 18, 2026 at 11:43 AM Kuroda, Hayato/黒田 隼人
<kuroda(dot)hayato(at)fujitsu(dot)com> 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