Re: [Proposal] Adding Log File Capability to pg_createsubscriber

Hi Gyan,

Thanks for working on the patch.

On Mon, 16 Mar 2026 at 04:53, Gyan Sreejith <gyan(dot)sreejith(at)gmail(dot)com> wrote:
>
>
>
> On Wed, Mar 11, 2026 at 6:05 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>>
>> On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <gyan(dot)sreejith(at)gmail(dot)com> wrote:
>> >
>> > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <euler(at)eulerto(dot)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().

But we are not checking the log level when we are writing the logs in
the logfiles.
Due to this, extra logs can appear.

>>
>>
>>
>> 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!
I think along with it we should also check the actual contents of each file.

>>
>>
>> 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.

I noticed that we do not define pg_log_warning, pg_log_warning_detail
and pg_log_warning_hint, due to this the contents when option
'--logdir' is specified and when it is not defined can differ. Should
we define these as well?

Also internal_log_file_write is declared as:
+static void
+ internal_log_file_write(const char *format,...)
__attribute__((format(printf, 1, 2)));
Should we use 'pg_attribute_printf' instead of
__attribute__((format(printf, 1, 2)))?

I have added the changes for above in the topup patch. I also did some
cosmetic changes.
I have also addressed the comment (2) by Shveta in [1].

If you agree with these changes, please feel free to include them in the patch.
[1]: https://www.postgresql.org/message-id/CAJpy0uBPvz6S9VE8sLYmoju4BGYh94uks%2BUTocPdD094xqmZ2w%40mail.gmail.com

Thanks,
Shlok Kyal