On Wed, 15 May 2024 at 20:31, Robert Haas <robertmh...@gmail.com> wrote:
> That's fine, except that I think that what the previous discussion
> revealed is that we don't have consensus on how backtrace behavior
> ought to be controlled: backtrace_on_internal_error was one proposal,
> and this was a competing proposal, and neither one of them seemed to
> be completely satisfactory.
Attached is a rebased patchset of my previous proposal, including a
few changes that Michael preferred:
1. Renames log_backtrace to log_backtrace_mode
2. Rename internal to internal_error
I reread the thread since I previously posted the patch and apart from
Michaels feedback I don't think there was any more feedback on the
current proposal.
Rethinking about it myself though, I think the main downside of this
proposal is that if you want the previous behaviour of
backtrace_functions (add backtraces to all elog/ereports in the given
functions) you now need to set three GUCs:
log_backtrace_mode='all'
backtrace_functions='some_func'
backtrace_min_level=DEBUG5
The third one is not needed in the common case where someone only
cares about errors, but still needing to set log_backtrace_mode='all'
might seem a bit annoying. One way around that would be to make
log_backtrace_mode and backtrace_functions be additive instead of
subtractive.
Personally I think the proposed subtractive nature would be exactly
what I want for backtraces I'm interested in. Because I would want to
use backtrace_functions in this way:
1. I see an error I want a backtrace of: et log_backtrace_mode='all'
and try to trigger again.
2. Oops, there's many backtraces now let's filter by function: set
backtrace_functions=some_func
So if it's additive, I'd have to also undo log_backtrace_mode='all'
again at step 2. So changing two GUCs instead of one to do what I
want.
From 85a894cc32381a4ff2a1c7aab31476b2bbf6bdf3 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <jelte.fennema@microsoft.com>
Date: Thu, 21 Mar 2024 13:05:35 +0100
Subject: [PATCH 1/2] Add PGErrorVerbosity to typedefs.list
This one was missing, resulting in some strange alignment.
---
src/include/utils/elog.h | 2 +-
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 054dd2bf62f..da1a7469fa5 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -493,7 +493,7 @@ typedef enum
PGERROR_TERSE, /* single-line error messages */
PGERROR_DEFAULT, /* recommended style */
PGERROR_VERBOSE, /* all the facts, ma'am */
-} PGErrorVerbosity;
+} PGErrorVerbosity;
extern PGDLLIMPORT int Log_error_verbosity;
extern PGDLLIMPORT char *Log_line_prefix;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 61ad417cde6..67ec5408a98 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1778,6 +1778,7 @@ PGAsyncStatusType
PGCALL2
PGChecksummablePage
PGContextVisibility
+PGErrorVerbosity
PGEvent
PGEventConnDestroy
PGEventConnReset
base-commit: 3e53492aa7084bceaa92757c90e067d79768797e
--
2.34.1
From 9a1256aa3e4f585e3f588122662050f2302585dc Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <jelte.fennema@microsoft.com>
Date: Thu, 27 Jun 2024 10:56:10 +0200
Subject: [PATCH 2/2] Allow logging backtraces in more cases
We previously only had the backtrace_functions GUC to control when
backtraces were logged. Based on mailinglist discussion there were two
common cases that people wanted backtraces that were not covered by this
GUC though:
1. Logging backtraces for all internal errors
2. Logging backtraces for all errors
To support those two usecases, as well as to allow users to continue to
log backtraces for specific warnings using `backtrace_functions`, this
modifies the GUCs in the following way:
1. Adds a `log_backtrace` GUC, which can be set to `none` (default),
`internal_error` and `all` to log different types of backtraces.
2. Change `backtrace_functions` to behave as an additional filter on top
of `log_backtrace`. The empty string (the default) is now interpreted
as doing no filtering based on function name.
3. Add a `backtrace_min_level` GUC, which limits for which log entries
backtraces are written, based on their log level. This defaults to
ERROR.
This does mean that setting `backtrace_functions=some_func` alone is not
enough to get backtraces for some_func. You now need to combine that
with `log_backtrace_mode=all` and if you want to get backtraces for
non-errors (which you previously got), you also need to change
backtrace_min_level to whichever level you want backtraces for.
---
doc/src/sgml/config.sgml | 82 +++++++++++++++++--
src/backend/utils/error/elog.c | 30 ++++++-
src/backend/utils/misc/guc_tables.c | 50 +++++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/utils/elog.h | 8 ++
src/include/utils/guc.h | 1 +
src/tools/pgindent/typedefs.list | 1 +
7 files changed, 162 insertions(+), 11 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 0c7a9082c54..ff2222ef79c 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7283,6 +7283,44 @@ local0.* /var/log/postgresql
</listitem>
</varlistentry>
+ <varlistentry id="guc-log-backtrace-mode" xreflabel="log_backtrace_mode">
+ <term><varname>log_backtrace_mode</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>log_backtrace_mode</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ If this parameter is set to <literal>all</literal> then for all log
+ entries a backtrace is written to the server log together with the log
+ message. If this parameter is set to <literal>internal_error</literal> then
+ such a backtrace is only written for logs with error code XX000
+ (internal error; see also <xref linkend="errcodes-appendix"/>).
+ This can be used to debug such internal errors (which should normally
+ not happen in production). Finally, if this parameter is set to
+ <literal>none</literal> (the default), no backtraces are ever written
+ to the server log.
+ </para>
+
+ <para>
+ The logs for which a backtrace is written can be further restricted
+ using <xref linkend="guc-backtrace-min-level"/> (default:
+ <literal>ERROR</literal>) and <xref linkend="guc-backtrace-functions"/>
+ (default: empty string, meaning all).
+ </para>
+
+ <para>
+ Backtrace support is not available on all platforms, and the quality
+ of the backtraces depends on compilation options.
+ </para>
+
+ <para>
+ Only superusers and users with the appropriate <literal>SET</literal>
+ privilege can change this setting.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-log-checkpoints" xreflabel="log_checkpoints">
<term><varname>log_checkpoints</varname> (<type>boolean</type>)
<indexterm>
@@ -11372,16 +11410,45 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</term>
<listitem>
<para>
- This parameter contains a comma-separated list of C function names.
- If an error is raised and the name of the internal C function where
- the error happens matches a value in the list, then a backtrace is
- written to the server log together with the error message. This can
- be used to debug specific areas of the source code.
+ This parameter can contain a comma-separated list of C function names,
+ which can be used to filter for which logs a backtrace is written to
+ the server log.
+ If a log entry is raised and the name of the
+ internal C function where the error happens does not match any of the
+ values in the list, then no backtrace is written to the server log.
+ This can be used to only debug specific areas of the source code.
</para>
<para>
- Backtrace support is not available on all platforms, and the quality
- of the backtraces depends on compilation options.
+ The empty string (the default) disables any such filtering. So for any
+ logs that match both <xref linkend="guc-log-backtrace-mode"/> and
+ <xref linkend="guc-backtrace-min-level"/> a backtrace is
+ written to the server log.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="guc-backtrace-min-level" xreflabel="backtrace_min_level">
+ <term><varname>backtrace_min_level</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>backtrace_min_level</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Controls which <link linkend="runtime-config-severity-levels">message
+ levels</link> cause backtraces to be written to the log, for log
+ messages that match both <xref linkend="guc-log-backtrace-mode"/> and
+ <xref linkend="guc-backtrace-functions"/>.
+ Valid values are <literal>DEBUG5</literal>, <literal>DEBUG4</literal>,
+ <literal>DEBUG3</literal>, <literal>DEBUG2</literal>, <literal>DEBUG1</literal>,
+ <literal>LOG</literal>, <literal>INFO</literal>, <literal>NOTICE</literal>,
+ <literal>WARNING</literal>, <literal>ERROR</literal>, <literal>FATAL</literal>,
+ and <literal>PANIC</literal>. Each level includes all the levels that
+ follow it. The later the level, the fewer messages result in a
+ backtrace. The default is <literal>ERROR</literal>. Note that
+ <literal>LOG</literal> has a different rank here than in
+ <xref linkend="guc-log-min-messages"/>.
</para>
<para>
@@ -11391,6 +11458,7 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</listitem>
</varlistentry>
+
<varlistentry id="guc-debug-discard-caches" xreflabel="debug_discard_caches">
<term><varname>debug_discard_caches</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d91a85cb2d7..08ee899f6bc 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -111,6 +111,7 @@ int Log_error_verbosity = PGERROR_DEFAULT;
char *Log_line_prefix = NULL; /* format for extra log line info */
int Log_destination = LOG_DESTINATION_STDERR;
char *Log_destination_string = NULL;
+int log_backtrace = LOGBACKTRACE_NONE;
bool syslog_sequence_numbers = true;
bool syslog_split_messages = true;
@@ -181,6 +182,7 @@ static void set_stack_entry_domain(ErrorData *edata, const char *domain);
static void set_stack_entry_location(ErrorData *edata,
const char *filename, int lineno,
const char *funcname);
+static bool matches_backtrace_gucs(ErrorData *edata);
static bool matches_backtrace_functions(const char *funcname);
static pg_noinline void set_backtrace(ErrorData *edata, int num_skip);
static void set_errdata_field(MemoryContextData *cxt, char **ptr, const char *str);
@@ -496,10 +498,7 @@ errfinish(const char *filename, int lineno, const char *funcname)
oldcontext = MemoryContextSwitchTo(ErrorContext);
/* Collect backtrace, if enabled and we didn't already */
- if (!edata->backtrace &&
- edata->funcname &&
- backtrace_functions &&
- matches_backtrace_functions(edata->funcname))
+ if (!edata->backtrace && matches_backtrace_gucs(edata))
set_backtrace(edata, 2);
/*
@@ -819,6 +818,26 @@ set_stack_entry_location(ErrorData *edata,
edata->funcname = funcname;
}
+/*
+ * matches_backtrace_gucs --- checks whether the log entry matches
+ * log_backtrace_mode, backtrace_min_level and backtrace_functions.
+ */
+static bool
+matches_backtrace_gucs(ErrorData *edata)
+{
+ if (log_backtrace == LOGBACKTRACE_NONE)
+ return false;
+
+ if (log_backtrace == LOGBACKTRACE_INTERNAL_ERROR &&
+ edata->sqlerrcode != ERRCODE_INTERNAL_ERROR)
+ return false;
+
+ if (backtrace_min_level > edata->elevel)
+ return false;
+
+ return matches_backtrace_functions(edata->funcname);
+}
+
/*
* matches_backtrace_functions --- checks whether the given funcname matches
* backtrace_functions
@@ -830,6 +849,9 @@ matches_backtrace_functions(const char *funcname)
{
const char *p;
+ if (!backtrace_functions || backtrace_functions[0] == '\0')
+ return true;
+
if (!backtrace_function_list || funcname == NULL || funcname[0] == '\0')
return false;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 46c258be282..87826a32cfb 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -163,6 +163,23 @@ static const struct config_enum_entry server_message_level_options[] = {
{NULL, 0, false}
};
+static const struct config_enum_entry backtrace_level_options[] = {
+ {"debug5", DEBUG5, false},
+ {"debug4", DEBUG4, false},
+ {"debug3", DEBUG3, false},
+ {"debug2", DEBUG2, false},
+ {"debug1", DEBUG1, false},
+ {"debug", DEBUG2, true},
+ {"log", LOG, false},
+ {"info", INFO, true},
+ {"notice", NOTICE, false},
+ {"warning", WARNING, false},
+ {"error", ERROR, false},
+ {"fatal", FATAL, false},
+ {"panic", PANIC, false},
+ {NULL, 0, false}
+};
+
static const struct config_enum_entry intervalstyle_options[] = {
{"postgres", INTSTYLE_POSTGRES, false},
{"postgres_verbose", INTSTYLE_POSTGRES_VERBOSE, false},
@@ -200,6 +217,16 @@ static const struct config_enum_entry log_error_verbosity_options[] = {
StaticAssertDecl(lengthof(log_error_verbosity_options) == (PGERROR_VERBOSE + 2),
"array length mismatch");
+static const struct config_enum_entry log_backtrace_options[] = {
+ {"none", LOGBACKTRACE_NONE, false},
+ {"internal_error", LOGBACKTRACE_INTERNAL_ERROR, false},
+ {"all", LOGBACKTRACE_ALL, false},
+ {NULL, 0, false}
+};
+
+StaticAssertDecl(lengthof(log_backtrace_options) == (LOGBACKTRACE_ALL + 2),
+ "array length mismatch");
+
static const struct config_enum_entry log_statement_options[] = {
{"none", LOGSTMT_NONE, false},
{"ddl", LOGSTMT_DDL, false},
@@ -531,6 +558,7 @@ int log_temp_files = -1;
double log_statement_sample_rate = 1.0;
double log_xact_sample_rate = 0;
char *backtrace_functions;
+int backtrace_min_level = ERROR;
int temp_file_limit = -1;
@@ -4724,6 +4752,18 @@ struct config_enum ConfigureNamesEnum[] =
NULL, NULL, NULL
},
+ {
+ {"backtrace_min_level", PGC_SUSET, DEVELOPER_OPTIONS,
+ gettext_noop("Sets the message levels that create backtraces when log_backtrace is configured."),
+ gettext_noop("Each level includes all the levels that follow it. The later"
+ " the level, the fewer backtraces are created."),
+ GUC_NOT_IN_SAMPLE
+ },
+ &backtrace_min_level,
+ ERROR, backtrace_level_options,
+ NULL, NULL, NULL
+ },
+
{
{"bytea_output", PGC_USERSET, CLIENT_CONN_STATEMENT,
gettext_noop("Sets the output format for bytea."),
@@ -4820,6 +4860,16 @@ struct config_enum ConfigureNamesEnum[] =
NULL, NULL, NULL
},
+ {
+ {"log_backtrace", PGC_SUSET, LOGGING_WHAT,
+ gettext_noop("Sets if logs should include a backtrace."),
+ NULL
+ },
+ &log_backtrace,
+ LOGBACKTRACE_NONE, log_backtrace_options,
+ NULL, NULL, NULL
+ },
+
{
{"log_error_verbosity", PGC_SUSET, LOGGING_WHAT,
gettext_noop("Sets the verbosity of logged messages."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e0567de2190..33bde793cdc 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -575,6 +575,7 @@
# their durations, > 0 logs only
# actions running at least this number
# of milliseconds.
+#log_backtrace = 'none'
#log_checkpoints = on
#log_connections = off
#log_disconnections = off
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index da1a7469fa5..c6796cf0265 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -495,9 +495,17 @@ typedef enum
PGERROR_VERBOSE, /* all the facts, ma'am */
} PGErrorVerbosity;
+typedef enum
+{
+ LOGBACKTRACE_NONE, /* no backtrace */
+ LOGBACKTRACE_INTERNAL_ERROR, /* backtrace for internal error code */
+ LOGBACKTRACE_ALL, /* backtrace for all logs */
+} LogBacktraceVerbosity;
+
extern PGDLLIMPORT int Log_error_verbosity;
extern PGDLLIMPORT char *Log_line_prefix;
extern PGDLLIMPORT int Log_destination;
+extern PGDLLIMPORT int log_backtrace;
extern PGDLLIMPORT char *Log_destination_string;
extern PGDLLIMPORT bool syslog_sequence_numbers;
extern PGDLLIMPORT bool syslog_split_messages;
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index ff506bf48d9..8417f203891 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -267,6 +267,7 @@ extern PGDLLIMPORT int log_temp_files;
extern PGDLLIMPORT double log_statement_sample_rate;
extern PGDLLIMPORT double log_xact_sample_rate;
extern PGDLLIMPORT char *backtrace_functions;
+extern PGDLLIMPORT int backtrace_min_level;
extern PGDLLIMPORT int temp_file_limit;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 67ec5408a98..15c596e33a7 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1531,6 +1531,7 @@ LockTupleMode
LockViewRecurse_context
LockWaitPolicy
LockingClause
+LogBacktraceVerbosity
LogOpts
LogStmtLevel
LogicalDecodeBeginCB
--
2.34.1
