Thread: relcache leak warnings vs. errors

I just fixed a relcache leak that I accidentally introduced 
(5a1d0c9925).  Because it was a TAP test involving replication workers, 
you don't see the usual warning anywhere unless you specifically check 
the log files manually.

How about a compile-time option to turn all the warnings in resowner.c 
into errors?  This could be enabled automatically by --enable-cassert, 
similar to other defines that that option enables.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Apr 11, 2020 at 10:09:59AM +0200, Peter Eisentraut wrote:
> I just fixed a relcache leak that I accidentally introduced (5a1d0c9925).
> Because it was a TAP test involving replication workers, you don't see the
> usual warning anywhere unless you specifically check the log files manually.
> 
> How about a compile-time option to turn all the warnings in resowner.c into
> errors?  This could be enabled automatically by --enable-cassert, similar to
> other defines that that option enables.
> 

+1.  Would it be worthwhile to do the same in e.g. aset.c (for
MEMORY_CONTEXT_CHECKING case), or more generally for stuff in
src/backend/utils?

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> How about a compile-time option to turn all the warnings in resowner.c 
> into errors?  This could be enabled automatically by --enable-cassert, 
> similar to other defines that that option enables.

[ itch... ]  Those calls occur post-commit; throwing an error there
is really a mess, which is why it's only WARNING now.

I guess you could make them PANICs, but it would be an option that nobody
could possibly want to have enabled in anything resembling production.
So I"m kind of -0.5 on making --enable-cassert do it automatically.
Although I suppose that it's not really worse than other assertion
failures.

            regards, tom lane

Hi,

On 2020-04-11 10:54:49 -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > How about a compile-time option to turn all the warnings in resowner.c 
> > into errors?  This could be enabled automatically by --enable-cassert, 
> > similar to other defines that that option enables.
> 
> [ itch... ]  Those calls occur post-commit; throwing an error there
> is really a mess, which is why it's only WARNING now.

> I guess you could make them PANICs, but it would be an option that nobody
> could possibly want to have enabled in anything resembling production.
> So I"m kind of -0.5 on making --enable-cassert do it automatically.
> Although I suppose that it's not really worse than other assertion
> failures.

I'd much rather see this throw an assertion than the current
behaviour. But I'm wondering if there's a chance we can throw an error
in non-assert builds without adding too much complexity to the error
paths. Could we perhaps throw the error a bit later during the commit
processing?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2020-04-11 10:54:49 -0400, Tom Lane wrote:
>> I guess you could make them PANICs, but it would be an option that nobody
>> could possibly want to have enabled in anything resembling production.
>> So I"m kind of -0.5 on making --enable-cassert do it automatically.
>> Although I suppose that it's not really worse than other assertion
>> failures.

> I'd much rather see this throw an assertion than the current
> behaviour. But I'm wondering if there's a chance we can throw an error
> in non-assert builds without adding too much complexity to the error
> paths. Could we perhaps throw the error a bit later during the commit
> processing?

Any error post-commit is a semantic disaster.

I guess that an assertion wouldn't be so awful, if people would rather
do it like that in debug builds.

            regards, tom lane

On Mon, Apr 13, 2020 at 04:22:26PM -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> I'd much rather see this throw an assertion than the current
>> behaviour. But I'm wondering if there's a chance we can throw an error
>> in non-assert builds without adding too much complexity to the error
>> paths. Could we perhaps throw the error a bit later during the commit
>> processing?
>
> Any error post-commit is a semantic disaster.

Yes, I can immediately think of two problems in the very recent
history where this has bitten.

> I guess that an assertion wouldn't be so awful, if people would rather
> do it like that in debug builds.

WARNING is useful mainly for tests where the output is checked, like
the main regression test suite.  Now that TAP scenarios get more and
more complex, +1 on the addition of an assertion for a hard failure.
I don't think either that's worth controlling with a developer GUC.
--
Michael