Re: [Proposal] Adding Log File Capability to pg_createsubscriber

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)

2) Instead of just checking if the file is created or not, let's check
for some contents from the file:
+# Check that the log files were created
+my @server_log_files = glob "$logdir/*/pg_createsubscriber_server.log";
+is( scalar(@server_log_files), 1, "
+    pg_createsubscriber_server.log file was created");
+my @internal_log_files = glob "$logdir/*/pg_createsubscriber_internal.log";
+is( scalar(@internal_log_files), 1, "
+    pg_createsubscriber_internal.log file was created");

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";

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);
}

Regards,
Vignesh