Re: Adding some error context for lock wait failures

Hi,

On 2025-10-09 13:33:44 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2025-10-09 12:50:53 -0400, Tom Lane wrote:
> >> Concretely, like the attached. This passes check-world, but
> >> I can't test it under valgrind because I'm hitting the same
> >> CREATE DATABASE failure skink is reporting.
>
> > Sorry, was working on a fix when life rudely intervened. Here's a quick
> > temporary fix:
>
> Thanks. With that, I've confirmed that this change suppresses the
> leak report in your example, and it also gets through the core
> regression tests under valgrind (though I didn't run leak checking
> for that). That's enough to convince me that the fix is OK.

There are a few places that do
ereport(...);

/* Flush any strings created in ErrorContext */
FlushErrorState();

That'd be obsoleted by this change, right?

I also looked through other mentions of ErrorContext to see if there are any
issues, didn't find anything.

There are some independently worrisome functions I noticed while looking
around for problems. E.g. initTrie() and GetConnection() catching errors
without doing a transaction rollback seems decidedly not great. But that's
unrelated, just noticed it while grepping around.

> Do you have an opinion on whether to back-patch? I'm leaning
> in the direction of doing so, but it could be argued that it's
> too much risk for a problem that we only know for sure exists
> in master.

I'm a bit worried about it causing problems, although I don't have an actual
theory as to how. So I'd slightly lean towards not backpatching. But it's just
a vague gut feeling.

Greetings,

Andres Freund