Improve frontend error logging style.

Get rid of the separate "FATAL" log level, as it was applied
so inconsistently as to be meaningless.  This mostly involves
s/pg_log_fatal/pg_log_error/g.

Create a macro pg_fatal() to handle the common use-case of
pg_log_error() immediately followed by exit(1).  Various
modules had already invented either this or equivalent macros;
standardize on pg_fatal() and apply it where possible.

Invent the ability to add "detail" and "hint" messages to a
frontend message, much as we have long had in the backend.

Except where rewording was needed to convert existing coding
to detail/hint style, I have (mostly) resisted the temptation
to change existing message wording.

Patch by me.  Design and patch reviewed at various stages by
Robert Haas, Kyotaro Horiguchi, Peter Eisentraut and
Daniel Gustafsson.

Discussion: https://postgr.es/m/1363732.1636496441@sss.pgh.pa.us

2 files changed, 98 insertions, 20 deletions

diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index 61cfdce6531..e213bb70d0a 100644
--- a/src/include/common/logging.h
+++ b/src/include/common/logging.h
@@ -16,7 +16,7 @@
 enum pg_log_level
 {
 	/*
-	 * Not initialized yet
+	 * Not initialized yet (not to be used as an actual message log level).
 	 */
 	PG_LOG_NOTSET = 0,
 
@@ -43,19 +43,41 @@ enum pg_log_level
 	PG_LOG_ERROR,
 
 	/*
-	 * Severe errors that cause program termination.  (One-shot programs may
-	 * chose to label even fatal errors as merely "errors".  The distinction
-	 * is up to the program.)
+	 * Turn all logging off (not to be used as an actual message log level).
 	 */
-	PG_LOG_FATAL,
+	PG_LOG_OFF,
+};
+
+/*
+ * __pg_log_level is the minimum log level that will actually be shown.
+ */
+extern enum pg_log_level __pg_log_level;
 
+/*
+ * A log message can have several parts.  The primary message is required,
+ * others are optional.  When emitting multiple parts, do so in the order of
+ * this enum, for consistency.
+ */
+enum pg_log_part
+{
 	/*
-	 * Turn all logging off.
+	 * The primary message.  Try to keep it to one line; follow the backend's
+	 * style guideline for primary messages.
 	 */
-	PG_LOG_OFF,
-};
+	PG_LOG_PRIMARY,
 
-extern PGDLLIMPORT enum pg_log_level __pg_log_level;
+	/*
+	 * Additional detail.  Follow the backend's style guideline for detail
+	 * messages.
+	 */
+	PG_LOG_DETAIL,
+
+	/*
+	 * Hint (not guaranteed correct) about how to fix the problem.  Follow the
+	 * backend's style guideline for hint messages.
+	 */
+	PG_LOG_HINT,
+};
 
 /*
  * Kind of a hack to be able to produce the psql output exactly as required by
@@ -70,27 +92,84 @@ void		pg_logging_increase_verbosity(void);
 void		pg_logging_set_pre_callback(void (*cb) (void));
 void		pg_logging_set_locus_callback(void (*cb) (const char **filename, uint64 *lineno));
 
-void		pg_log_generic(enum pg_log_level level, const char *pg_restrict fmt,...) pg_attribute_printf(2, 3);
-void		pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap) pg_attribute_printf(2, 0);
+void		pg_log_generic(enum pg_log_level level, enum pg_log_part part,
+						   const char *pg_restrict fmt,...)
+			pg_attribute_printf(3, 4);
+void		pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
+							 const char *pg_restrict fmt, va_list ap)
+			pg_attribute_printf(3, 0);
+
+/*
+ * Preferred style is to use these macros to perform logging; don't call
+ * pg_log_generic[_v] directly, except perhaps in error interface code.
+ */
+#define pg_log_error(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_ERROR)) \
+			pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+	} while(0)
 
-#define pg_log_fatal(...) do { \
-		if (likely(__pg_log_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
+#define pg_log_error_detail(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_ERROR)) \
+			pg_log_generic(PG_LOG_ERROR, PG_LOG_DETAIL, __VA_ARGS__); \
 	} while(0)
 
-#define pg_log_error(...) do { \
-		if (likely(__pg_log_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __VA_ARGS__); \
+#define pg_log_error_hint(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_ERROR)) \
+			pg_log_generic(PG_LOG_ERROR, PG_LOG_HINT, __VA_ARGS__); \
 	} while(0)
 
 #define pg_log_warning(...) do { \
-		if (likely(__pg_log_level <= PG_LOG_WARNING)) pg_log_generic(PG_LOG_WARNING, __VA_ARGS__); \
+		if (likely(__pg_log_level <= PG_LOG_WARNING)) \
+			pg_log_generic(PG_LOG_WARNING, PG_LOG_PRIMARY, __VA_ARGS__); \
+	} while(0)
+
+#define pg_log_warning_detail(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_WARNING)) \
+			pg_log_generic(PG_LOG_WARNING, PG_LOG_DETAIL, __VA_ARGS__); \
+	} while(0)
+
+#define pg_log_warning_hint(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_WARNING)) \
+			pg_log_generic(PG_LOG_WARNING, PG_LOG_HINT, __VA_ARGS__); \
 	} while(0)
 
 #define pg_log_info(...) do { \
-		if (likely(__pg_log_level <= PG_LOG_INFO)) pg_log_generic(PG_LOG_INFO, __VA_ARGS__); \
+		if (likely(__pg_log_level <= PG_LOG_INFO)) \
+			pg_log_generic(PG_LOG_INFO, PG_LOG_PRIMARY, __VA_ARGS__); \
+	} while(0)
+
+#define pg_log_info_detail(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_INFO)) \
+			pg_log_generic(PG_LOG_INFO, PG_LOG_DETAIL, __VA_ARGS__); \
+	} while(0)
+
+#define pg_log_info_hint(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_INFO)) \
+			pg_log_generic(PG_LOG_INFO, PG_LOG_HINT, __VA_ARGS__); \
 	} while(0)
 
 #define pg_log_debug(...) do { \
-		if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) pg_log_generic(PG_LOG_DEBUG, __VA_ARGS__); \
+		if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
+			pg_log_generic(PG_LOG_DEBUG, PG_LOG_PRIMARY, __VA_ARGS__); \
+	} while(0)
+
+#define pg_log_debug_detail(...) do { \
+		if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
+			pg_log_generic(PG_LOG_DEBUG, PG_LOG_DETAIL, __VA_ARGS__); \
+	} while(0)
+
+#define pg_log_debug_hint(...) do { \
+		if (unlikely(__pg_log_level <= PG_LOG_DEBUG)) \
+			pg_log_generic(PG_LOG_DEBUG, PG_LOG_HINT, __VA_ARGS__); \
+	} while(0)
+
+/*
+ * A common shortcut: pg_log_error() and immediately exit(1).
+ */
+#define pg_fatal(...) do { \
+		if (likely(__pg_log_level <= PG_LOG_ERROR)) \
+			pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+		exit(1); \
 	} while(0)
 
 #endif							/* COMMON_LOGGING_H */
diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
index 81929270100..4a3d0ec2c55 100644
--- a/src/include/lib/simplehash.h
+++ b/src/include/lib/simplehash.h
@@ -293,8 +293,7 @@ SH_SCOPE void SH_STAT(SH_TYPE * tb);
 #define SIMPLEHASH_H
 
 #ifdef FRONTEND
-#define sh_error(...) \
-	do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0)
+#define sh_error(...) pg_fatal(__VA_ARGS__)
 #define sh_log(...) pg_log_info(__VA_ARGS__)
 #else
 #define sh_error(...) elog(ERROR, __VA_ARGS__)