Hi Gyan, Thanks for working on the patch.
On Mon, 16 Mar 2026 at 04:53, Gyan Sreejith <[email protected]> wrote: > > > > On Wed, Mar 11, 2026 at 6:05 AM vignesh C <[email protected]> wrote: >> >> On Tue, 10 Mar 2026 at 04:26, Gyan Sreejith <[email protected]> wrote: >> > >> > On Thu, Mar 5, 2026 at 9:49 AM Euler Taveira <[email protected]> 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
topup.patch
Description: Binary data
v9-0001-Add-a-new-argument-l-logdir-to-pg_createsubscribe.patch
Description: Binary data
