How to best find unexpected WARNINGs in the TAP tests?
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253531psql -h localhost -U postgresBuilt from patchset v1 (message #1), August 26, 2026 at 01:01 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253531_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253531_1 && git checkout t253531_1Patchset v1 (message #1) is on t253531_1
In [1]/messages/by-id/OS9PR01MB121491E7E05950D108AF9A6D8F5A72@OS9PR01MB12149.jpnprd01.prod.outlook.com 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 only glanced at the TAP test module code. I see we have a
teardown_node function, which maybe could be coded to look for certain
WARNINGs in the log and fail due to those.
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. 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.
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.
Anyway, I'm not particularly excited about the BUG_WARNING idea, but I
am hoping someone has some better ideas. I'm currently a little
frustrated at the tests having encountered this problem for 10 months
without having told us about it...
David
(You may need to apply [2]/messages/by-id/attachment/202173/v20-0001-Close-relations-opened-specifically-for-AFTER-tr.patch to get the attached patch to pass all tests.)
[1]: /messages/by-id/OS9PR01MB121491E7E05950D108AF9A6D8F5A72@OS9PR01MB12149.jpnprd01.prod.outlook.com
[2]: /messages/by-id/attachment/202173/v20-0001-Close-relations-opened-specifically-for-AFTER-tr.patch
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
#endifThen 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
On Tue, 25 Aug 2026 at 00:43, Andres Freund <andres@anarazel.de> wrote:
On 2026-08-25 00:27:27 +1200, David Rowley wrote:
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.
I guess we could PANIC at the end of the context checking functions if
we got > 0 WARNINGS, but how much more informative is that over just
doing a PANIC if *any* of the checks fail? Do we really need the full
list of issues?
Maybe we could have a cassert GUC like bool panic_on_bug_warning, then
give BUG_WARNING a distinct number and add USE_ASSERT_CHECKING code to
errstart() to promote BUG_WARNIGNs to PANICs when that GUC is true or
set them to WARNING when false. We could then modify the TAP test to
always set panic_on_bug_warning to true.
David