Simplify backtrace_functions GUC code

From 0e07942e20076ad0f6b6ae62f9cc82a48d6971ba Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sun, 17 Mar 2024 05:42:26 +0000
Subject: [PATCH v1] Simplify backtrace_functions GUC code

---
 src/backend/utils/error/elog.c      | 124 ++++++++++++++--------------
 src/backend/utils/misc/guc_tables.c |   4 +-
 src/include/utils/guc_hooks.h       |   1 -
 3 files changed, 65 insertions(+), 64 deletions(-)

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 52bc01058c..40b2dfc00e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -114,9 +114,6 @@ char	   *Log_destination_string = NULL;
 bool		syslog_sequence_numbers = true;
 bool		syslog_split_messages = true;
 
-/* Processed form of backtrace_functions GUC */
-static char *backtrace_function_list;
-
 #ifdef HAVE_SYSLOG
 
 /*
@@ -830,22 +827,42 @@ set_stack_entry_location(ErrorData *edata,
 static bool
 matches_backtrace_functions(const char *funcname)
 {
-	const char *p;
+	char	   *rawstring;
+	List	   *elemlist;
+	ListCell   *l;
+	bool		is_parasing_okay PG_USED_FOR_ASSERTS_ONLY;
+
 
-	if (!backtrace_function_list || funcname == NULL || funcname[0] == '\0')
+	if (backtrace_functions == NULL || backtrace_functions[0] == '\0' ||
+		funcname == NULL || funcname[0] == '\0')
 		return false;
 
-	p = backtrace_function_list;
-	for (;;)
+	/* Need a modifiable copy of string */
+	rawstring = pstrdup(backtrace_functions);
+
+	/* Parse string into list of identifiers */
+	is_parasing_okay = SplitIdentifierString(rawstring, ',', &elemlist);
+
+	/*
+	 * We already have parsed the list in check_backtrace_functions, just
+	 * assert the fact here.
+	 */
+	Assert(is_parasing_okay);
+
+	foreach(l, elemlist)
 	{
-		if (*p == '\0')			/* end of backtrace_function_list */
-			break;
+		char	   *tok = (char *) lfirst(l);
 
-		if (strcmp(funcname, p) == 0)
+		if (strcmp(tok, funcname) == 0)
+		{
+			pfree(rawstring);
+			list_free(elemlist);
 			return true;
-		p += strlen(p) + 1;
+		}
 	}
 
+	pfree(rawstring);
+	list_free(elemlist);
 	return false;
 }
 
@@ -2124,68 +2141,53 @@ DebugFileOpen(void)
 bool
 check_backtrace_functions(char **newval, void **extra, GucSource source)
 {
-	int			newvallen = strlen(*newval);
-	char	   *someval;
-	int			validlen;
-	int			i;
-	int			j;
+	char	   *rawstring;
+	List	   *elemlist;
+	ListCell   *l;
 
-	/*
-	 * Allow characters that can be C identifiers and commas as separators, as
-	 * well as some whitespace for readability.
-	 */
-	validlen = strspn(*newval,
-					  "0123456789_"
-					  "abcdefghijklmnopqrstuvwxyz"
-					  "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-					  ", \n\t");
-	if (validlen != newvallen)
+	/* Need a modifiable copy of string */
+	rawstring = pstrdup(*newval);
+
+	/* Parse string into list of identifiers */
+	if (!SplitIdentifierString(rawstring, ',', &elemlist))
 	{
-		GUC_check_errdetail("Invalid character");
+		/* syntax error in list */
+		GUC_check_errdetail("List syntax is invalid.");
+		pfree(rawstring);
+		list_free(elemlist);
 		return false;
 	}
 
-	if (*newval[0] == '\0')
+	foreach(l, elemlist)
 	{
-		*extra = NULL;
-		return true;
-	}
+		char	   *tok = (char *) lfirst(l);
+		int			toklen = strlen(tok);
+		int			validlen;
 
-	/*
-	 * Allocate space for the output and create the copy.  We could discount
-	 * whitespace chars to save some memory, but it doesn't seem worth the
-	 * trouble.
-	 */
-	someval = guc_malloc(ERROR, newvallen + 1 + 1);
-	for (i = 0, j = 0; i < newvallen; i++)
-	{
-		if ((*newval)[i] == ',')
-			someval[j++] = '\0';	/* next item */
-		else if ((*newval)[i] == ' ' ||
-				 (*newval)[i] == '\n' ||
-				 (*newval)[i] == '\t')
-			;					/* ignore these */
-		else
-			someval[j++] = (*newval)[i];	/* copy anything else */
-	}
+		/*
+		 * Allow characters that can be C identifiers, commas as separators,
+		 * the wildcard symbol, as well as some whitespace for readability.
+		 */
+		validlen = strspn(tok,
+						  "0123456789_"
+						  "abcdefghijklmnopqrstuvwxyz"
+						  "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+						  ", \n\t");
 
-	/* two \0s end the setting */
-	someval[j] = '\0';
-	someval[j + 1] = '\0';
+		if (validlen != toklen)
+		{
+			GUC_check_errdetail("Invalid character");
+			pfree(rawstring);
+			list_free(elemlist);
+			return false;
+		}
+	}
 
-	*extra = someval;
+	pfree(rawstring);
+	list_free(elemlist);
 	return true;
 }
 
-/*
- * GUC assign_hook for backtrace_functions
- */
-void
-assign_backtrace_functions(const char *newval, void *extra)
-{
-	backtrace_function_list = (char *) extra;
-}
-
 /*
  * GUC check_hook for log_destination
  */
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 57d9de4dd9..7e9d1f7f5f 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4652,11 +4652,11 @@ struct config_string ConfigureNamesString[] =
 		{"backtrace_functions", PGC_SUSET, DEVELOPER_OPTIONS,
 			gettext_noop("Log backtrace for errors in these functions."),
 			NULL,
-			GUC_NOT_IN_SAMPLE
+			GUC_LIST_INPUT | GUC_NOT_IN_SAMPLE
 		},
 		&backtrace_functions,
 		"",
-		check_backtrace_functions, assign_backtrace_functions, NULL
+		check_backtrace_functions, NULL, NULL
 	},
 
 	{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index d64dc5fcdb..b9b801216a 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -37,7 +37,6 @@ extern bool check_vacuum_buffer_usage_limit(int *newval, void **extra,
 											GucSource source);
 extern bool check_backtrace_functions(char **newval, void **extra,
 									  GucSource source);
-extern void assign_backtrace_functions(const char *newval, void *extra);
 extern bool check_bonjour(bool *newval, void **extra, GucSource source);
 extern bool check_canonical_path(char **newval, void **extra, GucSource source);
 extern void assign_checkpoint_completion_target(double newval, void *extra);
-- 
2.34.1