Subject: Re: Support a wildcard in backtrace_functions

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 28 Feb 2024 19:50:19 +0100

On Wed, 28 Feb 2024 at 19:04, Daniel Gustafsson <[email-masked]> wrote:
> This should be "equal to or higher" right?

Correct, nicely spotted. Fixed that. I also updated the docs for the
new backtrace_functions_min_level GUC itself too, as well as creating
a dedicated options array for the GUC. Because when updating its docs
I realized that none of the existing level arrays matched what we
wanted.
From 5ab507155a1cb60e490209d28a092fc2d7c0f6be Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 20 Dec 2023 11:41:18 +0100
Subject: [PATCH v4 2/2] Add wildcard support to backtrace_functions GUC

With this change setting backtrace_functions to '*' will start logging
backtraces for all errors (or more precisely all logs).
---
 doc/src/sgml/config.sgml       | 5 +++++
 src/backend/utils/error/elog.c | 8 +++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index bc066284334..2a6d015d6d4 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11275,6 +11275,11 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
         code.
        </para>
 
+       <para>
+        A single <literal>*</literal> character is interpreted as a wildcard and
+        will cause all errors in the log to contain backtraces.
+       </para>
+
        <para>
         Backtrace support is not available on all platforms, and the quality
         of the backtraces depends on compilation options.
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 699d9d0a241..553e4785520 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -843,6 +843,8 @@ matches_backtrace_functions(const char *funcname)
 		if (*p == '\0')			/* end of backtrace_function_list */
 			break;
 
+		if (strcmp("*", p) == 0)
+			return true;
 		if (strcmp(funcname, p) == 0)
 			return true;
 		p += strlen(p) + 1;
@@ -2133,14 +2135,14 @@ check_backtrace_functions(char **newval, void **extra, GucSource source)
 	int			j;
 
 	/*
-	 * Allow characters that can be C identifiers and commas as separators, as
-	 * well as some whitespace for readability.
+	 * Allow characters that can be C identifiers, commas as separators, the
+	 * wildcard symbol, as well as some whitespace for readability.
 	 */
 	validlen = strspn(*newval,
 					  "0123456789_"
 					  "abcdefghijklmnopqrstuvwxyz"
 					  "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-					  ", \n\t");
+					  ",* \n\t");
 	if (validlen != newvallen)
 	{
 		GUC_check_errdetail("Invalid character");
-- 
2.34.1

From d928c3e49593f3bf08def8a40c246d3aa8de46ac Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 27 Feb 2024 17:31:24 +0100
Subject: [PATCH v4 1/2] Add backtrace_functions_min_level

Before this change a stacktrace would be attached to all logs messages
in a function that matched backtrace_functions. But in most cases people
only care about the stacktraces for messages with an ERROR level. This
changes that default to only log stacktraces for ERROR messages but
keeps the option open of logging stacktraces for different log levels
too by having users configure the new backtrace_functions_min_level GUC.
---
 doc/src/sgml/config.sgml            | 46 ++++++++++++++++++++++++++---
 src/backend/utils/error/elog.c      |  1 +
 src/backend/utils/misc/guc_tables.c | 29 ++++++++++++++++++
 src/include/utils/guc.h             |  1 +
 4 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 43b1a132a2c..bc066284334 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11267,10 +11267,12 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       <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.
+        If a log entry is raised with a level equal to or higher than
+        <xref linkend="guc-backtrace-functions-min-level"/> 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.
        </para>
 
        <para>
@@ -11285,6 +11287,42 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-backtrace-functions-min-level" xreflabel="backtrace_functions_min_level">
+      <term><varname>backtrace_functions_min_level</varname> (<type>string</type>)
+      <indexterm>
+        <primary><varname>backtrace_functions_min_level</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Controls which <link linkend="runtime-config-severity-levels">message
+        levels</link> cause stack traces to be written to the log, for messages
+        raised from C functions that match the configured
+        <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>
+        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-backtrace-on-internal-error" xreflabel="backtrace_on_internal_error">
       <term><varname>backtrace_on_internal_error</varname> (<type>boolean</type>)
       <indexterm>
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index c9719f358b6..699d9d0a241 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -500,6 +500,7 @@ errfinish(const char *filename, int lineno, const char *funcname)
 	if (!edata->backtrace &&
 		((edata->funcname &&
 		  backtrace_functions &&
+		  edata->elevel >= backtrace_functions_min_level &&
 		  matches_backtrace_functions(edata->funcname)) ||
 		 (edata->sqlerrcode == ERRCODE_INTERNAL_ERROR &&
 		  backtrace_on_internal_error)))
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 93ded31ed92..94497e0174d 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -162,6 +162,23 @@ static const struct config_enum_entry server_message_level_options[] = {
 	{NULL, 0, false}
 };
 
+static const struct config_enum_entry backtrace_functions_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},
@@ -530,6 +547,7 @@ double		log_statement_sample_rate = 1.0;
 double		log_xact_sample_rate = 0;
 char	   *backtrace_functions;
 bool		backtrace_on_internal_error = false;
+int			backtrace_functions_min_level = ERROR;
 
 int			temp_file_limit = -1;
 
@@ -4689,6 +4707,17 @@ struct config_enum ConfigureNamesEnum[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"backtrace_functions_min_level", PGC_SUSET, DEVELOPER_OPTIONS,
+			gettext_noop("Sets the message levels that create backtraces when backtrace_functions is configured"),
+			NULL,
+			GUC_NOT_IN_SAMPLE
+		},
+		&backtrace_functions_min_level,
+		ERROR, backtrace_functions_level_options,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"bytea_output", PGC_USERSET, CLIENT_CONN_STATEMENT,
 			gettext_noop("Sets the output format for bytea."),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 471d53da8f0..69d5cb7a125 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -267,6 +267,7 @@ extern PGDLLIMPORT double log_statement_sample_rate;
 extern PGDLLIMPORT double log_xact_sample_rate;
 extern PGDLLIMPORT char *backtrace_functions;
 extern PGDLLIMPORT bool backtrace_on_internal_error;
+extern PGDLLIMPORT int backtrace_functions_min_level;
 
 extern PGDLLIMPORT int temp_file_limit;

base-commit: 53c2a97a92665be6bd7d70bd62ae6158fe4db96e
-- 
2.34.1

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 29 Feb 2024 11:12:00 +0100

> On 28 Feb 2024, at 19:50, Jelte Fennema-Nio <[email-masked]> wrote:
> 

Looks good, I'm marking this ready for committer for now.  I just have a few
small comments:

+        A single <literal>*</literal> character is interpreted as a wildcard and
+        will cause all errors in the log to contain backtraces.
This should mention that it's all error matching the level (or higher) of the
newly introduced GUC.

+	gettext_noop("Sets the message levels that create backtraces when backtrace_functions is configured"),
I think we should add the same "Each level includes.." long_desc, and the
short_desc should end with period.

+       <para>
+        Backtrace support is not available on all platforms, and the quality
+        of the backtraces depends on compilation options.
+       </para>
I don't think we need to duplicate this para here, having it on
backtrace_functions suffice.

--
Daniel Gustafsson

---

From: Alvaro Herrera <[email-masked]>
Date: Thu, 29 Feb 2024 11:35:21 +0100

>  		if (strcmp(funcname, p) == 0)

Hmm, so if I write "foo,*" this will work but check all function names
first and on the second entry.  But if I write "foo*" the GUC value will
be accepted but match nothing (as will "*foo" or "foo*bar").  I don't
like either of these behaviors.  I think we should tighten this up: an
asterisk should be allowed only if it appears alone in the string
(short-circuiting check_backtrace_functions before strspn); and let's
leave the strspn() call alone.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 6 Mar 2024 00:18:14 +0530

> leave the strspn() call alone.

+1 for disallowing *foo or foo* or foo*bar etc. combinations. I think
we need to go a bit further and convert backtrace_functions of type
GUC_LIST_INPUT so that check_backtrace_functions can just use
SplitIdentifierString to parse the list of identifiers. Then, the
strspn can just be something like below for each token:

        validlen = strspn(*tok,
                        "0123456789_"
                        "abcdefghijklmnopqrstuvwxyz"
                        "ABCDEFGHIJKLMNOPQRSTUVWXYZ");

Does anyone see a problem with it?

FWIW, I've recently noticed for my work on
https://commitfest.postgresql.org/47/2863/ that there isn't any test
covering all the backtrace related code - backtrace_functions GUC,
backtrace_on_internal_error GUC, set_backtrace(), backtrace(),
backtrace_symbols(). I came up with a test module covering these areas
https://commitfest.postgresql.org/47/4823/. I could make the TAP tests
pass on all the CF bot animals. Interestingly, the new code that gets
added for this thread can also be covered with it. Any thoughts are
welcome.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com