Обсуждение: Assert Levels

Currently we have only Assert(), or a run-time test.

Can we introduce levels of assertion? That way we can choose how
paranoid a build to make, like setting log_min_messages.

We know many Assertions are costly, so we don't usually do performance
tests with --enable-cassert. But then we may not notice assertion
failures on those tests for rare failures.

There are also a few run-time tests that "never happen", so perhaps
those could be introduced as a first level of assertion. Production
builds would likely to continue to be built with those tests enabled.

We might also want to have a special log level for such failures, so
people know to report them if they occur, e.g. TELL.

It would also allow us a smoother move into production: gradually reduce
assertion checking over time as software matures.

Anyway, fairly handwavy stuff and I doubt those specific ideas are
useful, but the general train of thought may lead somewhere.

Thoughts?

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:
> Can we introduce levels of assertion?

The thing that is good about Assert() is that it doesn't require a lot
of programmer effort to put one in.  I'm not in favor of complexifying
it.
        regards, tom lane

greg

On 19 Sep 2008, at 13:20, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Can we introduce levels of assertion?
>
> The thing that is good about Assert() is that it doesn't require a lot
> of programmer effort to put one in.  I'm not in favor of complexifying
> it.
>

Perhaps just an Assert_expensive() would be useful if someone wants to  
to the work of going through all the assertions and determining if  
they're especially expensive. We already have stuff like CLOBBER*.

You'll also have to do enough empirical tests to convince people that  
a --enable-cheap-casserts build really does perform the same as a  
regular build.




>            regards, tom lane
>
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Greg Stark <greg.stark@enterprisedb.com> writes:
> You'll also have to do enough empirical tests to convince people that  
> a --enable-cheap-casserts build really does perform the same as a  
> regular build.

I don't think performance is even the main issue.  We have never
recommended having Asserts on in production because they can decrease
system-wide reliability.  As per the fine manual:
The assertion checks are not categorized for severity,and so what might be a relatively harmless bug will still leadto
serverrestarts if it triggers an assertion failure.
 

        regards, tom lane

greg

On 19 Sep 2008, at 20:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Greg Stark <greg.stark@enterprisedb.com> writes:
>> You'll also have to do enough empirical tests to convince people that
>> a --enable-cheap-casserts build really does perform the same as a
>> regular build.
>
> I don't think performance is even the main issue.  We have never
> recommended having Asserts on in production because they can decrease
> system-wide reliability.  As per the fine manual:

I think Simon's coming at this from a different angle - at least this  
is what I took out of his suggestion: there Might be bugs or "can't  
happen" events that can happen but only under heavy load. Currently  
anyone running heavy workloads does it with assertions off so we  
aren't testing that case.

There was a case of this that the HOT thesting turned up. There was a  
"can't happen" assertion failure related to hint bits being lost that  
was happening.

This is a good example of why running with assertions enabled on  
production might not be a good idea. But it's also a good example of  
why we should do our performance testing with assertions enabled if we  
can do it without invalidating the results. 

On Fri, 19 Sep 2008, Greg Stark wrote:

> This is a good example of why running with assertions enabled on production 
> might not be a good idea. But it's also a good example of why we should do 
> our performance testing with assertions enabled if we can do it without 
> invalidating the results.

The performance impact of assertions is large enough that I don't think 
that goal is practical.  However, issues like this do suggest that running 
tests under a very heavy load like those used for performance testing 
should sometimes be done with assertions on just to shake bugs out, even 
if the actual performance results will be tossed.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD

Greg Smith <gsmith@gregsmith.com> writes:
> On Fri, 19 Sep 2008, Greg Stark wrote:
>> This is a good example of why running with assertions enabled on production 
>> might not be a good idea. But it's also a good example of why we should do 
>> our performance testing with assertions enabled if we can do it without 
>> invalidating the results.

> The performance impact of assertions is large enough that I don't think 
> that goal is practical.

Well, there are certain things that --enable-cassert turns on that are
outrageously expensive; notably CLOBBER_FREED_MEMORY and
MEMORY_CONTEXT_CHECKING.  It wouldn't be too unreasonable to decouple
those things somehow (with a means more accessible than editing
pg_config_manual.h).

I don't think anyone knows what the performance impact of just the
regular Asserts is; it's been too long since these other things were
stuck in there.
        regards, tom lane

On Fri, 2008-09-19 at 17:33 -0400, Greg Smith wrote:
> On Fri, 19 Sep 2008, Greg Stark wrote:
> 
> > This is a good example of why running with assertions enabled on production 
> > might not be a good idea. But it's also a good example of why we should do 
> > our performance testing with assertions enabled if we can do it without 
> > invalidating the results.
> 
> The performance impact of assertions is large enough 

The starting point of this was that the *current* performance impact of
assertions is large enough that we turn them off.

> that I don't think that goal is practical. 

So we can't use that as an argument against doing something to enable
the lighter checks in certain circumstances in the future.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

On Fri, 2008-09-19 at 17:47 -0400, Tom Lane wrote:
> Greg Smith <gsmith@gregsmith.com> writes:
> > On Fri, 19 Sep 2008, Greg Stark wrote:
> >> This is a good example of why running with assertions enabled on production 
> >> might not be a good idea. But it's also a good example of why we should do 
> >> our performance testing with assertions enabled if we can do it without 
> >> invalidating the results.
> 
> > The performance impact of assertions is large enough that I don't think 
> > that goal is practical.
> 
> Well, there are certain things that --enable-cassert turns on that are
> outrageously expensive; notably CLOBBER_FREED_MEMORY and
> MEMORY_CONTEXT_CHECKING.  It wouldn't be too unreasonable to decouple
> those things somehow (with a means more accessible than editing
> pg_config_manual.h).

That's mostly what I'm hoping for. If we call the CLOBBER checks as
class 3, all current Asserts as class 2 then we can invent a class 1 of
specifically lightweight checks (only). We can then have
--enable-cassert=X rather than just y or n

> I don't think anyone knows what the performance impact of just the
> regular Asserts is; it's been too long since these other things were
> stuck in there.

Agreed. If we had a system in place, we would slowly move towards using
it. It wouldn't happen overnight, but it would help. Many more people
might be persuaded to be early adopters if there was an extra mode in
place to catch problems. 

My thinking was that I want to put lots of checks into Hot Standby to
prove it all works, but in places where I don't think they're really
needed.

An example of a current set of checks we do, that may not be needed are
the tests for HeapTupleInvisible in HeapTupleSatisfiesUpdate(). Almost
every time we do a TransactionIdIsInProgress() to see if the heap tuple
is invisible. If it is we throw an ERROR. But we already did that
earlier. Now I've never seen that ERROR reported anywhere, so I'm
thinking that I'd like to downgrade that somehow, yet still retain the
ability to check it when things go strange. There are a few other
examples.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2008-09-19 at 17:47 -0400, Tom Lane wrote:
>> Well, there are certain things that --enable-cassert turns on that are
>> outrageously expensive; notably CLOBBER_FREED_MEMORY and
>> MEMORY_CONTEXT_CHECKING.  It wouldn't be too unreasonable to decouple
>> those things somehow (with a means more accessible than editing
>> pg_config_manual.h).

> That's mostly what I'm hoping for. If we call the CLOBBER checks as
> class 3, all current Asserts as class 2 then we can invent a class 1 of
> specifically lightweight checks (only). We can then have
> --enable-cassert=X rather than just y or n

Hold on a minute.  I don't mind refactoring the way that configure
controls those existing build switches.  I do object to complexifying
routine uses of Assert when absolutely zero evidence of a benefit has
been presented. How do you know that the run-of-the-mill Asserts aren't
lightweight enough already?

> An example of a current set of checks we do, that may not be needed are
> the tests for HeapTupleInvisible in HeapTupleSatisfiesUpdate().

Yes, they are needed, think about concurrent updates: sure the tuple
must have been visible awhile back, but we haven't been holding
exclusive lock on its buffer continuously since then.  There might be
some places where test-and-elog checks could be downgraded to
assertions, but I would tread very very carefully in that.  If the
original code author was sure it "couldn't happen" he'd have written it
as an Assert to begin with.
        regards, tom lane

On Sat, 2008-09-20 at 11:28 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Fri, 2008-09-19 at 17:47 -0400, Tom Lane wrote:
> >> Well, there are certain things that --enable-cassert turns on that are
> >> outrageously expensive; notably CLOBBER_FREED_MEMORY and
> >> MEMORY_CONTEXT_CHECKING.  It wouldn't be too unreasonable to decouple
> >> those things somehow (with a means more accessible than editing
> >> pg_config_manual.h).
> 
> > That's mostly what I'm hoping for. If we call the CLOBBER checks as
> > class 3, all current Asserts as class 2 then we can invent a class 1 of
> > specifically lightweight checks (only). We can then have
> > --enable-cassert=X rather than just y or n
> 
> Hold on a minute.  I don't mind refactoring the way that configure
> controls those existing build switches.  I do object to complexifying
> routine uses of Assert when absolutely zero evidence of a benefit has
> been presented. How do you know that the run-of-the-mill Asserts aren't
> lightweight enough already?

Well, we don't. That's why I'd suggest to do it slowly and classify
everything as medium weight until proven otherwise. Also think we need
to take code location into account, because a cheap test in a critical
place could end up costing more than an expensive test that hardly ever
gets executed.

Anyway, if we do it at all, I think this probably should be classified
as code cleanup and done later in release cycle. If you think it's a
good idea after a couple of months we can start on it.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs wrote:
> Well, we don't. That's why I'd suggest to do it slowly and classify
> everything as medium weight until proven otherwise.

Once you have classified all asserts, what do we do with the result? 
What would be the practical impact?  What would be your recommendation 
about who runs with what setting?

Peter Eisentraut <peter_e@gmx.net> writes:
> Simon Riggs wrote:
>> Well, we don't. That's why I'd suggest to do it slowly and classify
>> everything as medium weight until proven otherwise.

> Once you have classified all asserts, what do we do with the result? 
> What would be the practical impact?  What would be your recommendation 
> about who runs with what setting?

Being able to keep asserts on while doing performance stress testing
was the suggested use case.  I think we'd still recommend having them
off in production.

FWIW, my gut feeling about it is that 99% of the asserts in the backend
are lightweight, ie, have no meaningful effect on performance.  There
are a small number that are expensive (the tests on validity of List
structures come to mind, as well as what we already discussed).  I don't
have any more evidence for this than Simon has for his "they're mostly
medium-weight" assumption, but I'd point out that by definition most of
the backend code isn't performance-critical.  So I think that an option
to turn off a few particularly expensive asserts would be sufficient.
Moreover, the more asserts you turn off, the less useful it would be to
do testing of this type.  I see essentially no value in a mode that
turns off the majority of assertions.
        regards, tom lane

Greg Smith <gsmith@gregsmith.com> writes:
> The next time I'm doing some performance testing I'll try to quantify how 
> much damage the expensive ones do by playing with pg_config_manual.h. 
> Normally I'm testing with 1GB+ of shared_buffers which makes the current 
> assert scheme unusable.

There is a commit-time scan of the buffers for the sole purpose of
asserting a few things about their state.  That's one of the things
we'd need to turn off for a "cheap asserts only" mode I think.

If you want to try to quantify what "cheap asserts" might do, you
should pull out the #ifdef USE_ASSERT_CHECKING code here:check_list_invariants in list.cthe loops in AtEOXact_Buffers
andAtEOXact_LocalBuffersthe loop in AtEOXact_CatCachethe test that makes AtEOXact_RelationCache scan the relcache
evenwhen !need_eoxact_work
 
in addition to the memory-related stuff.
        regards, tom lane

On Fri, 19 Sep 2008, Tom Lane wrote:

> Well, there are certain things that --enable-cassert turns on that are 
> outrageously expensive...I don't think anyone knows what the performance 
> impact of just the regular Asserts is; it's been too long since these 
> other things were stuck in there.

The next time I'm doing some performance testing I'll try to quantify how 
much damage the expensive ones do by playing with pg_config_manual.h. 
Normally I'm testing with 1GB+ of shared_buffers which makes the current 
assert scheme unusable.  If I could run with asserts on only only lose a 
small amount of performance just by disabling the clobber and context 
checking, that would be valuable to know.  Right now I waste a fair amount 
of time running performance and assert builds in parallel.

--
* Greg Smith gsmith@gregsmith.com http://www.gregsmith.com Baltimore, MD