Re: Support a wildcard in backtrace_functions

On Fri, 22 Mar 2024 at 11:14, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> A few comments:
>
> 1.
> @@ -832,6 +849,9 @@ matches_backtrace_functions(const char *funcname)
> {
> const char *p;
>
> + if (!backtrace_functions || backtrace_functions[0] == '\0')
> + return true;
> +
>
> Shouldn't this be returning false to not log set backtrace when
> backtrace_functions is not set? Am I missing something here?

Empty string is considered the new wildcard, i.e. backtrace_functions
filtering is not enabled if it is empty. This seemed reasonable to me
since you should now disable backtraces by using log_backtrace=none,
having backtrace_functions='' mean the same thing seems rather
useless. I also documented this in the updated docs.

> 2.
> + {
> + {"log_backtrace", PGC_SUSET, LOGGING_WHAT,
> + gettext_noop("Sets if logs should include a backtrace."),
> + NULL
>
> IMV, log_backtrace, backtrace_min_level and backtrace_functions are
> interlinked, so keeping all of them as DEVELOPER_OPTIONS looks fine to
> me than having log_backtrace at just LOGGING_WHAT kind. Also, we must
> also mark log_backtrace as GUC_NOT_IN_SAMPLE.

I agree they are linked, but I don't think it's just useful for
developers to be able to set log_backtrace to internal (even if we
choose not to make "internal" the default).

> 3. I think it's worth adding a few examples to get backtraces in docs.
> For instance, what to set to get backtraces of all internal errors and
> what to set to get backtraces of all ERRORs coming from a specific
> function etc.

Good idea, I'll try to add those later. For now your specific cases would be:
log_backtrace = 'internal' (default in 0003)
backtrace_functions = '' (default)
backtrace_min_level = 'ERROR' (default)

and

log_backtrace = 'all'
backtrace_functions = 'someFunc'
backtrace_min_level = 'ERROR' (default)

> 4. I see the support for wildcard in backtrace_functions is missing.
> Is it intentionally left out? If not, can you make it part of 0003
> patch?

Yes it's intentional, see answer on 1.

> 5. The amount of backtraces generated is going to be too huge when
> setting log_backtrace = 'all' and backtrace_min_level = 'debug5'. With
> this setting installcheck generated 12MB worth of log and the test
> took about 55 seconds (as opposed to 48 seconds without it) The point
> is if these settings are misused, it can easily slow down the entire
> system and fill up disk space leading to crashes eventually. This
> makes a strong case for marking log_backtrace a developer only
> function.

I think the same argument can be made for many other GUCs that are not
marked as developer options (e.g. log_min_messages). Normally we
"protect" such options by using PGC_SUSET. DEVELOPER_OPTIONS is really
only meant for options that are only useful for developers

> 6. In continuation to comment #5, does anyone need backtrace for
> elevels like debugX and LOG etc.? What's the use of the backtrace in
> such cases?

I think at least WARNING and NOTICE could be useful in practice, but I
agree LOG and DEBUGX seem kinda useless. But it seems kinda strange to
not have them in the list, especially given it is pretty much no
effort to support them too.