Re: [Proposal] Adding Log File Capability to pg_createsubscriber

On Wed, Mar 11, 2026 at 6:05 AM vignesh C <vignesh21@gmail.com> wrote:

> On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <gyan.sreejith@gmail.com>
> wrote:
> >
> > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <euler@eulerto.com> wrote:
> >
> >> Don't duplicate code. If you are reusing a function, my advice is to
> move it to
> >> src/common. You can always use "ifdef FRONTEND" to use the appropriate
> log
> >> message (elog/ereport vs pg_error, for example).
> >
> >
> > I have made all the changes except for this one, and I am deferring to
> Amit Kapila regarding the marks.
>
> Few comments:
> 1) You are not checking log level because of which the contents are
> logged irrespective of the log level:
> +#undef pg_log_info
> +#define pg_log_info(...) do{\
> +       if (internal_log_file_fp != NULL) \
> +               internal_log_file_write(__VA_ARGS__); \
> +       else \
> +               pg_log_generic(PG_LOG_INFO,PG_LOG_PRIMARY,__VA_ARGS__);\
> +} while(0)
> +
> +#undef pg_log_info_hint
> +#define pg_log_info_hint(...) do{\
> +       if (internal_log_file_fp != NULL) \
> +               internal_log_file_write(__VA_ARGS__); \
> +       else \
> +               pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);\
> +} while(0)
> +
> +#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 log level is passed to and checked by pg_log_generic_v() which is
called by pg_log_generic().

>
>
2) Instead of just checking if the file is created or not, let's check
> for some contents from the file:
>
Added checks to ensure that the log files are not empty, thanks!

>
> 3) This change is not required, let's remove this:
> --- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
> +++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
> @@ -13,7 +13,8 @@ program_help_ok('pg_createsubscriber');
>  program_version_ok('pg_createsubscriber');
>  program_options_handling_ok('pg_createsubscriber');
>
> -my $datadir = PostgreSQL::Test::Utils::tempdir;
> +my $datadir = PostgreSQL::Test::Utils::tempdir + "/datadir";

Fixed

>
>
4) No need of '{' as it is a single line statement
> if (opt->log_dir != NULL)
> {
> appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir,
> log_timestamp, SERVER_LOG_FILE_NAME);
> }
>
Fixed

Thank you! I have attached the changes.
Regards,
Gyan