Assert Levels

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.

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 lead
to server restarts 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.com
PostgreSQL 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.com
PostgreSQL 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.com
PostgreSQL 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

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

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.c
the loops in AtEOXact_Buffers and AtEOXact_LocalBuffers
the loop in AtEOXact_CatCache
the test that makes AtEOXact_RelationCache scan the relcache
even when !need_eoxact_work
in addition to the memory-related stuff.

regards, tom lane