I wrote:
> Hmm, interesting. Taking up my point #2, I'd been thinking about
> proposing that we convert
> pg_log_error("query failed: %s", PQerrorMessage(conn));
> pg_log_error("query was: %s", todo);
> to
> pg_log_error("query failed: %s", PQerrorMessage(conn));
> pg_log_error_detail("Query was: %s", todo);
After looking around a bit, I see that a lot of these add-on messages
are more nearly hints than details, so we'd probably better support
both those cases right off the bat.
To move things along a bit, here's a draft patch to logging.h/.c only
to implement what I'm envisioning. I don't think there's much point
in doing the per-call-site gruntwork until we have agreement on what
the API is, so this seems like enough for discussion.
(As a fervent hater of colorization, I don't have an opinion about
whether or how to colorize the "detail:" and "hint:" fragments.
But I'll happily take somebody else's adjustment to add that.)
regards, tom lane
diff --git a/src/common/logging.c b/src/common/logging.c
index fb1a9b1f87..84a4cdfd1f 100644
--- a/src/common/logging.c
+++ b/src/common/logging.c
@@ -151,6 +151,9 @@ pg_logging_init(const char *argv0)
}
}
+/*
+ * Change the logging flags.
+ */
void
pg_logging_config(int new_flags)
{
@@ -194,17 +197,19 @@ 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_log_generic(enum pg_log_level level, enum pg_log_part part,
+ const char *pg_restrict fmt,...)
{
va_list ap;
va_start(ap, fmt);
- pg_log_generic_v(level, fmt, ap);
+ pg_log_generic_v(level, part, fmt, ap);
va_end(ap);
}
void
-pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list ap)
+pg_log_generic_v(enum pg_log_level level, enum pg_log_part part,
+ const char *pg_restrict fmt, va_list ap)
{
int save_errno = errno;
const char *filename = NULL;
@@ -232,7 +237,8 @@ pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list a
fmt = _(fmt);
- if (!(log_flags & PG_LOG_FLAG_TERSE) || filename)
+ if (part == PG_LOG_PRIMARY &&
+ (!(log_flags & PG_LOG_FLAG_TERSE) || filename))
{
if (sgr_locus)
fprintf(stderr, ANSI_ESCAPE_FMT, sgr_locus);
@@ -251,30 +257,34 @@ pg_log_generic_v(enum pg_log_level level, const char *pg_restrict fmt, va_list a
if (!(log_flags & PG_LOG_FLAG_TERSE))
{
- switch (level)
+ switch (part)
{
- case PG_LOG_FATAL:
- if (sgr_error)
- fprintf(stderr, ANSI_ESCAPE_FMT, sgr_error);
- fprintf(stderr, _("fatal: "));
- if (sgr_error)
- fprintf(stderr, ANSI_ESCAPE_RESET);
- break;
- case PG_LOG_ERROR:
- if (sgr_error)
- fprintf(stderr, ANSI_ESCAPE_FMT, sgr_error);
- fprintf(stderr, _("error: "));
- if (sgr_error)
- fprintf(stderr, ANSI_ESCAPE_RESET);
+ case PG_LOG_PRIMARY:
+ switch (level)
+ {
+ case PG_LOG_ERROR:
+ if (sgr_error)
+ fprintf(stderr, ANSI_ESCAPE_FMT, sgr_error);
+ fprintf(stderr, _("error: "));
+ if (sgr_error)
+ fprintf(stderr, ANSI_ESCAPE_RESET);
+ break;
+ case PG_LOG_WARNING:
+ if (sgr_warning)
+ fprintf(stderr, ANSI_ESCAPE_FMT, sgr_warning);
+ fprintf(stderr, _("warning: "));
+ if (sgr_warning)
+ fprintf(stderr, ANSI_ESCAPE_RESET);
+ break;
+ default:
+ break;
+ }
break;
- case PG_LOG_WARNING:
- if (sgr_warning)
- fprintf(stderr, ANSI_ESCAPE_FMT, sgr_warning);
- fprintf(stderr, _("warning: "));
- if (sgr_warning)
- fprintf(stderr, ANSI_ESCAPE_RESET);
+ case PG_LOG_DETAIL:
+ fprintf(stderr, _("detail: "));
break;
- default:
+ case PG_LOG_HINT:
+ fprintf(stderr, _("hint: "));
break;
}
}
diff --git a/src/include/common/logging.h b/src/include/common/logging.h
index a71cf84249..ec589ccd03 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,20 +43,42 @@ 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.)
- */
- PG_LOG_FATAL,
-
- /*
- * Turn all logging off.
+ * Turn all logging off (not to be used as an actual message log level).
*/
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
+{
+ /*
+ * The primary message. Try to keep it to one line; follow the backend's
+ * style guideline for primary messages.
+ */
+ PG_LOG_PRIMARY,
+
+ /*
+ * 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
* the regression tests.
@@ -70,27 +92,85 @@ 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);
-#define pg_log_fatal(...) do { \
- if (likely(__pg_log_level <= PG_LOG_FATAL)) pg_log_generic(PG_LOG_FATAL, __VA_ARGS__); \
+/*
+ * 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_error(...) do { \
- if (likely(__pg_log_level <= PG_LOG_ERROR)) pg_log_generic(PG_LOG_ERROR, __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_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 special case: pg_log_error() and immediately exit(1). There is
+ * no situation where it makes sense to suppress the message, so we ignore
+ * __pg_log_level.
+ */
+#define pg_log_fatal(...) do { \
+ pg_log_generic(PG_LOG_ERROR, PG_LOG_PRIMARY, __VA_ARGS__); \
+ exit(1); \
} while(0)
#endif /* COMMON_LOGGING_H */
