kuroda_diffs_atop_v10.diffs

application/octet-stream

Filename: kuroda_diffs_atop_v10.diffs
Type: application/octet-stream
Part: 0
Message: RE: [Proposal] Adding Log File Capability to pg_createsubscriber
diff --git a/src/bin/pg_basebackup/pg_createsubscriber.c b/src/bin/pg_basebackup/pg_createsubscriber.c
index 4450842d2b5..4e7f14913bf 100644
--- a/src/bin/pg_basebackup/pg_createsubscriber.c
+++ b/src/bin/pg_basebackup/pg_createsubscriber.c
@@ -176,8 +176,8 @@ static char *pg_ctl_path = NULL;
 static char *pg_resetwal_path = NULL;
 
 static FILE *internal_log_file_fp = NULL;	/* File ptr to log all messages to */
-static char *log_timestamp = NULL;	/* Timestamp to be used in all log file
-									 * names */
+static char *logdir = NULL;					/* Directory log files are put
+											 * (if specified) */
 
 /* standby / subscriber data directory */
 static char *subscriber_dir = NULL;
@@ -186,67 +186,103 @@ static bool recovery_ended = false;
 static bool standby_running = false;
 static bool recovery_params_set = false;
 
+/*
+ * Use our own pg_log_ faimilies and pg_fatal, because we must output both
+ * stderr and log file (if specified).
+ */
+#undef pg_log_error
+#define pg_log_error(...)												\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__);			\
+		pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__);		\
+	} while (0)
+
+#undef pg_log_error_detail
+#define pg_log_error_detail(...)										\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__);			\
+		pg_log_generic(PG_LOG_ERROR, PG_LOG_DETAIL, __VA_ARGS__);		\
+	} while (0)
+
+#undef pg_log_error_hint
+#define pg_log_error_hint(...)											\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__);			\
+		pg_log_generic(PG_LOG_ERROR, PG_LOG_HINT, __VA_ARGS__);			\
+	} while (0)
+
 #undef pg_log_warning
-#define pg_log_warning(...) \
-	if (internal_log_file_fp != NULL) \
-		internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
-	pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__)
+#define pg_log_warning(...)												\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__);		\
+		pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__);	\
+	} while (0)
 
 #undef pg_log_warning_detail
-#define pg_log_warning_detail(...) \
-	if (internal_log_file_fp != NULL) \
-		internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
-	pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__)
+#define pg_log_warning_detail(...)										\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__);		\
+		pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__);		\
+	} while (0)
+
 
 #undef pg_log_warning_hint
-#define pg_log_warning_hint(...) \
-	if (internal_log_file_fp != NULL) \
-		internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__); \
-	pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__)
+#define pg_log_warning_hint(...)										\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_WARNING, __VA_ARGS__);		\
+		pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__);		\
+	} while (0)
 
 #undef pg_log_info
-#define pg_log_info(...) do { \
-	if (internal_log_file_fp != NULL) \
-		internal_log_file_write(PG_LOG_INFO, __VA_ARGS__); \
-	else \
-		pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
-} while(0)
+#define pg_log_info(...)												\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_INFO, __VA_ARGS__);			\
+		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(PG_LOG_INFO, __VA_ARGS__); \
-	else \
-		pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__); \
-} while(0)
+#define pg_log_info_hint(...)											\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_INFO, __VA_ARGS__);			\
+		pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__);			\
+	} while (0)
 
 #undef pg_log_debug
-#define pg_log_debug(...) do { \
-	if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) { \
-		if (internal_log_file_fp != NULL) \
-			internal_log_file_write(PG_LOG_DEBUG, __VA_ARGS__); \
-		else \
-			pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \
-	} \
-} while(0)
+#define pg_log_debug(...)												\
+	do {																\
+		if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) {					\
+			if (internal_log_file_fp != NULL)							\
+				internal_log_file_write(PG_LOG_DEBUG, __VA_ARGS__);		\
+			pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__);	\
+		}																\
+	} while (0)
 
 #undef pg_fatal
-#define pg_fatal(...) do { \
-	if (internal_log_file_fp != NULL) \
-		internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__); \
-	pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
-	exit(1); \
-} while(0)
+#define pg_fatal(...)													\
+	do {																\
+		if (internal_log_file_fp != NULL)								\
+			internal_log_file_write(PG_LOG_ERROR, __VA_ARGS__);			\
+		pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__);		\
+		exit(1);														\
+	} while (0)
 
 static void
 internal_log_file_write(enum pg_log_level level, const char *format,...)
 {
 	va_list		args;
 
-	if (level < __pg_log_level)
-		return;
+	Assert(internal_log_file_fp);
 
-	if (internal_log_file_fp == NULL)
+	/* Do nothing if log level is too low. */
+	if (level < __pg_log_level)
 		return;
 
 	va_start(args, format);
@@ -288,33 +324,12 @@ logfile_open(const char *filename, const char *mode)
 }
 
 static void
-make_dir(const char *dir)
-{
-	struct stat statbuf;
-
-	if (stat(dir, &statbuf) == 0)
-		return;
-
-	if (errno != ENOENT)
-		pg_fatal("could not stat directory \"%s\": %m", dir);
-
-	if (mkdir(dir, S_IRWXU) == 0)
-	{
-		pg_log_info("directory %s created", dir);
-		return;
-	}
-
-	pg_fatal("could not create log directory \"%s\": %m", dir);
-}
-
-static void
-make_output_dirs(const char *log_dir)
+make_output_dirs(const char *log_basedir)
 {
 	char		timestamp[128];
 	struct timeval tval;
 	time_t		now;
 	struct tm	tmbuf;
-	char		timestamp_dir[MAXPGPATH];
 	int			len;
 
 	/* Generate timestamp */
@@ -329,20 +344,20 @@ make_output_dirs(const char *log_dir)
 			 sizeof(timestamp) - strlen(timestamp), ".%03u",
 			 (unsigned int) (tval.tv_usec / 1000));
 
-	log_timestamp = pg_strdup(timestamp);
-
-	/* Create base directory (ignore if exists) */
-	make_dir(log_dir);
-
 	/* Build timestamp directory path */
-	len = snprintf(timestamp_dir, MAXPGPATH, "%s/%s", log_dir, timestamp);
+	len = snprintf(logdir, MAXPGPATH, "%s/%s", log_basedir, timestamp);
 
 	if (len >= MAXPGPATH)
 		pg_fatal("directory path for log files, %s/%s, is too long",
-				 log_dir, timestamp);
+				 logdir, timestamp);
+
+	/* Create base directory (ignore if exists) */
+	if (mkdir(log_basedir, S_IRWXU) < 0 && errno != EEXIST)
+		pg_fatal("could not create directory \"%s\": %m", log_basedir);
 
-	/* Create timestamp directory */
-	make_dir(timestamp_dir);
+	/* Create BASE_DIR/$timestamp */
+	if (mkdir(logdir, S_IRWXU) < 0)
+		pg_fatal("could not create directory \"%s\": %m", logdir);
 }
 
 /*
@@ -913,9 +928,13 @@ modify_subscriber_sysid(const struct CreateSubscriberOptions *opt)
 	else
 		pg_log_info("running pg_resetwal on the subscriber");
 
-
-	if (opt->log_dir != NULL)
-		out_file = psprintf("%s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
+	/*
+	 * Redirecting the output to the logfile if specified. Since the output
+	 * would be very short, around one line, we do not provide a separate file
+	 * for it; it's done as a part of the server log.
+	 */
+	if (logdir)
+		out_file = psprintf("%s/%s.log", logdir, SERVER_LOG_FILE_NAME);
 	else
 		out_file = DEVNULL;
 
@@ -1834,8 +1853,8 @@ start_standby_server(const struct CreateSubscriberOptions *opt, bool restricted_
 	if (restrict_logical_worker)
 		appendPQExpBufferStr(pg_ctl_cmd, " -o \"-c max_logical_replication_workers=0\"");
 
-	if (opt->log_dir != NULL)
-		appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s/%s.log", opt->log_dir, log_timestamp, SERVER_LOG_FILE_NAME);
+	if (logdir)
+		appendPQExpBuffer(pg_ctl_cmd, " -l %s/%s.log", logdir, SERVER_LOG_FILE_NAME);
 
 	pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data);
 	rc = system(pg_ctl_cmd->data);
@@ -2549,18 +2568,6 @@ main(int argc, char **argv)
 		}
 	}
 
-	if (opt.log_dir != NULL)
-	{
-		char	   *internal_log_file;
-
-		make_output_dirs(opt.log_dir);
-		internal_log_file = psprintf("%s/%s/%s.log", opt.log_dir, log_timestamp,
-									 INTERNAL_LOG_FILE_NAME);
-
-		if ((internal_log_file_fp = logfile_open(internal_log_file, "a")) == NULL)
-			pg_fatal("could not open log file \"%s\": %m", internal_log_file);
-	}
-
 	/* Validate that --all is not used with incompatible options */
 	if (opt.all_dbs)
 	{
@@ -2629,6 +2636,18 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
+	if (opt.log_dir != NULL)
+	{
+		char	   *internal_log_file;
+
+		make_output_dirs(opt.log_dir);
+		internal_log_file = psprintf("%s/%s.log", logdir,
+									 INTERNAL_LOG_FILE_NAME);
+
+		if ((internal_log_file_fp = logfile_open(internal_log_file)) == NULL)
+			pg_fatal("could not open log file \"%s\": %m", internal_log_file);
+	}
+
 	if (dry_run)
 		pg_log_info("Executing in dry-run mode.\n"
 					"The target directory will not be modified.");