Re: Support a wildcard in backtrace_functions

On Fri, Apr 19, 2024 at 2:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > There are some things that are pretty hard to change once we've
> > released them. For example, if we had a function or operator and
> > somebody embeds it in a view definition, removing or renaming it
> > prevents people from upgrading. But GUCs are not as bad.
>
> Really?  Are we certain that nobody will put SETs of this GUC
> into their applications, or even just activate it via ALTER DATABASE
> SET?  If they've done that, then a GUC change means dump/reload/upgrade
> failure.

That's fair. That feature isn't super-widely used, but it wouldn't be
crazy for someone to use it with this feature, either.

> I've not been following this thread, so I don't have an opinion
> about what the design ought to be.  But if we still aren't settled
> on it by now, I think the prudent thing is to revert the feature
> out of v17 altogether, and try again in v18.  When we're still
> designing something after feature freeze, that is a good indication
> that we are trying to ship something that's not ready for prime time.

There are two features at issue here. One is
backtrace_on_internal_error, committed as
a740b213d4b4d3360ad0cac696e47e5ec0eb8864. The other is a feature to
produce backtraces for all errors, which was originally proposed as
backtrace_functions='*', backtrace_functions_level=ERROR but which has
subsequently been proposed with a few other spellings that involve
merging that functionality into backtrace_on_internal_error. To the
extent that there's an open question here for PG17, it's not about
reverting this patch (which AIUI has never been committed) but about
reverting the earlier patch for backtrace_on_internal_error. So is
that the right thing to do?

Well, on the one hand, I confess to having had a passing thought that
backtrace_on_internal_error is awfully specific. Surely, user A's
criterion for which messages should have backtraces might be anything,
and we cannot reasonably add backtrace_on_$WHATEVER for all $WHATEVER
in some large set. And the debate here suggests that I wasn't the only
one to have that concern. So that argues for a revert.

But on the other hand, in my personal experience,
backtrace_on_internal_error would be the right thing in a really large
number of cases. I was disappointed to see it added as a developer
option with GUC_NOT_IN_SAMPLE. My vote would have been to put it in
postgresql.conf and enable it by default. We have a somewhat debatable
habit of using the exact same message in many places with similar
kinds of problems, and when a production system manages to hit one of
those errors, figuring out what actually went wrong is hard. In fact,
it's often hard even when the error text only occurs in one or two
places, because it's often some very low-level part of the code where
you can't get enough context to understand the problem without knowing
where that code got called from. So I sort of hate to see one of the
most useful extensions of backtrace functionality that I can
personally imagine get pulled back out of the tree because it turns
out that someone else has something else that they want.

I wonder whether a practical solution here might be to replace
backtrace_on_internal_error=true|false with
backtrace_on_error=true|internal|false. (Yes, I know that more
proposed resolutions is not necessarily what we need right now, but I
can't resist floating the idea.)

--
Robert Haas
EDB: http://www.enterprisedb.com