Re: How to best find unexpected WARNINGs in the TAP tests?

Hi,

On 2026-08-25 00:27:27 +1200, David Rowley wrote:
> In [1] there was a report about 39dcfda2d introducing a resource leak
> due to some missed table_close() calls. The 013_partition.pl triggered
> the issue, but we didn't get to find out about it because of how TAP
> tests work. A pg_regress test would fail as the psql output would show
> the WARNING, and we'd catch that with the diff against the expected
> result. With TAP, we don't do diffs like that, so this issue went
> unnoticed for (a rather embarrassing) 10 months.

I've always hated the fact these (and the aset.c etc "you corrupted memory"
ones) are WARNINGs, despite there clearly having been something bad to get to
that point.

Personally I regularly turn these kind of warnings into PANICs when working on
related code, as that's the only way to actually somewhat reliably be able to
find and debug problems.

> Another idea I thought about was to do:
>
> #ifdef USE_ASSERT_CHECKING
> #define BUG_WARNING ERROR
> #else
> #define BUG_WARNING WARNING
> #endif
>
> Then use BUG_WARNING in a select few places where we still want a
> WARNING in production builds, but we'd really like never to miss any
> of these in debug builds.

I'm not sure it's actually ok to use ERROR in all of these, because some of it
gets invoked in places like transaction abort that might not like getting
jumped out from willy-nilly.

> The attached patch does this and replaces a few WARNINGS with
> BUG_WARNING. This is only for the purpose of assisting discussion.

> The changes I made in the memory context code are a little strange as we
> only compile those functions in MEMORY_CONTEXT_CHECKING builds. I suspect
> these are WARNINGs today rather than ERRORs so we get a full damage report
> rather than just the first one.

Maybe, and I wouldn't be against emitting WARNINGs for anything we found and
then PANICing. But just carrying on with a WARNING continues to makes me feel
insane, 15+ years in.

> One annoying part of this is that to make test_resowner pass, I had to
> add not one, but two expected alternative outputs. I'm not yet quite
> clear on why the _2.out is needed. The order of the NOTICE and ERROR
> seems to alternate.

Huh. That seems like it needs some analysis.

Greetings,

Andres Freund