Hi,
While working on [1], I noticed that the backtrace_functions GUC code
does its own string parsing and uses another extra variable
backtrace_function_list to store the processed form of
backtrace_functions GUC.
I think the code can be simplified a bit by using
SplitIdentifierString like in the attached patch. With this,
backtrace_function_list variable and assign_backtrace_functions() will
go away.
Thoughts?
[1]
https://www.postgresql.org/message-id/CALj2ACXG1wzrb8%2B5HPNd8Qjr1h8GYkW-ijWhMYr2Y8_DzOB-%3Dg%40mail.gmail.com
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
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
