Re: [Proposal] Adding Log File Capability to pg_createsubscriber - Mailing list pgsql-hackers

On Tue, Feb 24, 2026, at 8:55 PM, Gyan Sreejith wrote:
> I have made the changes you suggested and have attached the patch below. 
>

[Avoid top-posting ...]

I took another look at this patch. Comments are below:

+/*
+ * Open a new logfile with proper permissions.
+ * From src/backend/postmaster/syslogger.c
+ */
+static FILE *
+logfile_open(const char *filename, const char *mode)
+{

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

-     * XXX this code was extracted from BootStrapXLOG().
+     * XXX this code was extracted from BootStrapXpg_log_info().

This is a typo.

+            case 'l':
+                {
+                    char        timestamp[128];
+                    struct timeval tval;
+                    time_t        now;
+                    struct tm    tmbuf;

I would expect to have this code in a function. That's the pattern it is using.

+                    strftime(timestamp, sizeof(timestamp), "%Y-%m-%d-%H-%M-%S", localtime_r(&now, &tmbuf));
+                    /* append microseconds */
+                    snprintf(timestamp + strlen(timestamp), sizeof(timestamp) - strlen(timestamp),
+                             ".%06u", (unsigned int) (tval.tv_usec));
+                    log_timestamp = pg_strdup(timestamp);

Do we really need microseconds? I would say milliseconds is sufficient. I
suggest to remove the dash (-); it is using a different style from existing
code (pg_upgrade).

+                    opt.log_dir = pg_strdup(optarg);
+                    canonicalize_path(opt.log_dir);

You didn't have a check for long path. It is not a good idea in general. See
MAXPGPATH examples. It would be a good idea if the path are restricted to
MAXPGPATH length.

+                        if (errno == ENOENT)
+                        {
+                            if (mkdir(opt.log_dir, S_IRWXU) == 0)
+                                pg_log_info("log directory created");
+                            else if (errno == EACCES)
+                                pg_fatal("permission denied trying to create log directory \"%s\": %m", opt.log_dir);
+                            else
+                                pg_fatal("could not create log directory \"%s\": %m", opt.log_dir);
+                        }
+                        else if (errno == EACCES)
+                            pg_fatal("permission denied trying to access directory \"%s\": %m", opt.log_dir);
+                        else
+                            pg_fatal("could not access directory \"%s\": %m", opt.log_dir);

The "permission denied" is redundant here because it will be in %m. Instead, I
suggest that you use

  could not create directory \"%s\": %m

The main advantage is that this sentence is already available. It avoids
translation effort.

+#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)

I don't like the fact that internal_log_file_fp is not declared before this
#define.

One of the arguments to have this feature was that pg_createsubscriber mixes the
server and tool messages. Couldn't we fix it adding "marks" on the output saying
the server log messages starts here and the server log messages ends here?


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/