| From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
|---|---|
| To: | PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | How to best find unexpected WARNINGs in the TAP tests? |
| Date: | 2026-08-24 12:27:27 |
| Message-ID: | CAApHDvp4Oi4EuNixzR-mynNi4mKpVNsvbCDkNA_-yEtu-BeA3w@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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 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] to get the attached patch to pass all tests.)
[1] https://postgr.es/m/OS9PR01MB121491E7E05950D108AF9A6D8F5A72@OS9PR01MB12149.jpnprd01.prod.outlook.com
[2] https://www.postgresql.org/message-id/attachment/202173/v20-0001-Close-relations-opened-specifically-for-AFTER-tr.patch
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-RFC-Add-BUG_WARNING-elevel.patch | application/octet-stream | 34.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andrey Borodin | 2026-08-24 12:31:39 | Re: Use streaming read I/O when enabling data checksums online |
| Previous Message | Sami Imseih | 2026-08-24 12:26:38 | Re: pgstat: Flush some statistics within running transactions, take 2 |