On Wed, Mar 6, 2024 at 12:41 AM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > > 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? > > IIRC the reason it's coded as it is, is so that we have a single palloc > chunk of memory to free when the value changes; we purposefully stayed > away from SplitIdentifierString and the like.
Why do we even need to prepare another list backtrace_function_list
from the parsed identifiers? Why can't we just do something like
v4-0003? Existing logic looks a bit complicated to me personally.
I still don't understand why we can't just turn backtrace_functions to
GUC_LIST_INPUT and use SplitIdentifierString? I see a couple of
advantages with this approach:
1. It simplifies the backtrace_functions GUC related code a lot.
2. We don't need assign_backtrace_functions() and a separate variable
backtrace_function_list, we can just rely on the GUC value
backtrace_functions.
3. All we do now in check_backtrace_functions() is just parse the user
entered backtrace_functions value, and quickly exit if we have found
'*'.
4. In matches_backtrace_functions(), we iterate over the list as we
already do right now.
With the v4-0003, all of the below test cases work:
ALTER SYSTEM SET backtrace_functions = 'pg_terminate_backend,
pg_create_restore_point';
SELECT pg_reload_conf();
SHOW backtrace_functions;
-- Must see backtrace
SELECT pg_create_restore_point(repeat('A', 1024));
-- Must see backtrace
SELECT pg_terminate_backend(1234, -1);
ALTER SYSTEM SET backtrace_functions = '*, pg_create_restore_point';
SELECT pg_reload_conf();
SHOW backtrace_functions;
-- Must see backtrace as * is specified
SELECT pg_terminate_backend(1234, -1);
-- Must see an error as * is specified in between the identifier name
ALTER SYSTEM SET backtrace_functions = 'pg*_create_restore_point';
ERROR: invalid value for parameter "backtrace_functions":
"pg*_create_restore_point"
DETAIL: Invalid character
> What problem do you see with the idea I proposed? That was:
>
> > On Thu, Feb 29, 2024 at 4:05 PM Alvaro Herrera <alvhe...@alvh.no-ip.org>
> > wrote:
>
> > > 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.
>
> That means, just add something like this at the top of
> check_backtrace_functions and don't do anything to this function
> otherwise (untested code):
>
> if (newval[0] == '*' && newval[1] == '\0')
> {
> someval = guc_malloc(ERROR, 2);
> if (someval == NULL)
> return false;
> someval[0] = '*';
> someval[1] = '\0';
> *extra = someval;
> return true;
> }
This works only if '* 'is specified as the only one character in
backtrace_functions = '*', right? If yes, what if someone sets
backtrace_functions = 'foo, bar, *, baz'?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
v4-0001-Add-backtrace_functions_min_level.patch
Description: Binary data
v4-0002-Add-wildcard-support-to-backtrace_functions-GUC.patch
Description: Binary data
v4-0003-Simplify-backtrace_functions-GUC-code.patch
Description: Binary data
