Thread: -DDISABLE_ENABLE_ASSERT
Hi, Since there seem to be multiple static checkers (coverity, clang checker) having problems with assert_enabled can we just optionally disable it? I am thinking of replacing it by a AssertionsEnabled() macro which then is unconditionally defined when DISABLE_ENABLE_ASSERT is defined. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> Since there seem to be multiple static checkers (coverity, clang
> checker) having problems with assert_enabled can we just optionally
> disable it?
> I am thinking of replacing it by a AssertionsEnabled() macro which then
> is unconditionally defined when DISABLE_ENABLE_ASSERT is defined.
We could do that ... but I wonder if we shouldn't remove assert_enabled
altogether. What's the use case for turning it off? Not matching the
speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING
doesn't get turned off.
If we went this direction I'd suggest keeping the GUC but turning it into
a read-only report of whether the backend was compiled with assertions.
regards, tom lane
On Thu, May 22, 2014 at 4:31 PM, Andres Freund <andres@2ndquadrant.com> wrote: > Since there seem to be multiple static checkers (coverity, clang > checker) having problems with assert_enabled can we just optionally > disable it? > I am thinking of replacing it by a AssertionsEnabled() macro which then > is unconditionally defined when DISABLE_ENABLE_ASSERT is defined. Uh, probably. But I think you need a better name; specifically, one that doesn't contain two words which are antonyms. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2014-05-22 16:37:35 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > Since there seem to be multiple static checkers (coverity, clang > > checker) having problems with assert_enabled can we just optionally > > disable it? > > I am thinking of replacing it by a AssertionsEnabled() macro which then > > is unconditionally defined when DISABLE_ENABLE_ASSERT is defined. > > We could do that ... but I wonder if we shouldn't remove assert_enabled > altogether. What's the use case for turning it off? Not matching the > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING > doesn't get turned off. I've used it once or twice to avoid having to recompile postgres when I wanted things not to be *that* slow (AtEOXactBuffers() I am looking at you). But I wouldn't be very sad if it'd go. Anybody against that? > If we went this direction I'd suggest keeping the GUC but turning it into > a read-only report of whether the backend was compiled with assertions. Yes, that makes sense. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund wrote: > On 2014-05-22 16:37:35 -0400, Tom Lane wrote: > > We could do that ... but I wonder if we shouldn't remove assert_enabled > > altogether. What's the use case for turning it off? Not matching the > > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING > > doesn't get turned off. > > I've used it once or twice to avoid having to recompile postgres when I > wanted things not to be *that* slow (AtEOXactBuffers() I am looking at > you). But I wouldn't be very sad if it'd go. > > Anybody against that? I have used it too (for a different reason IIRC), but like you I wouldn't have a problem if it weren't there. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, May 22, 2014 at 5:00 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Andres Freund wrote: >> On 2014-05-22 16:37:35 -0400, Tom Lane wrote: >> > We could do that ... but I wonder if we shouldn't remove assert_enabled >> > altogether. What's the use case for turning it off? Not matching the >> > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING >> > doesn't get turned off. >> >> I've used it once or twice to avoid having to recompile postgres when I >> wanted things not to be *that* slow (AtEOXactBuffers() I am looking at >> you). But I wouldn't be very sad if it'd go. >> >> Anybody against that? > > I have used it too (for a different reason IIRC), but like you I > wouldn't have a problem if it weren't there. I've used it, too, although not recently. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2014-05-23 07:20:12 -0400, Robert Haas wrote: > On Thu, May 22, 2014 at 5:00 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Andres Freund wrote: > >> On 2014-05-22 16:37:35 -0400, Tom Lane wrote: > >> > We could do that ... but I wonder if we shouldn't remove assert_enabled > >> > altogether. What's the use case for turning it off? Not matching the > >> > speed of a non-cassert build, because for instance MEMORY_CONTEXT_CHECKING > >> > doesn't get turned off. > >> > >> I've used it once or twice to avoid having to recompile postgres when I > >> wanted things not to be *that* slow (AtEOXactBuffers() I am looking at > >> you). But I wouldn't be very sad if it'd go. > >> > >> Anybody against that? > > > > I have used it too (for a different reason IIRC), but like you I > > wouldn't have a problem if it weren't there. > > I've used it, too, although not recently. That means you're for a (differently named) disable macro? Or is it not recent enough that you don't care? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, May 23, 2014 at 8:15 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> >> I've used it once or twice to avoid having to recompile postgres when I >> >> wanted things not to be *that* slow (AtEOXactBuffers() I am looking at >> >> you). But I wouldn't be very sad if it'd go. >> >> >> >> Anybody against that? >> > >> > I have used it too (for a different reason IIRC), but like you I >> > wouldn't have a problem if it weren't there. >> >> I've used it, too, although not recently. > > That means you're for a (differently named) disable macro? Or is it not > recent enough that you don't care? I'm leaning toward thinking we should just rip it out. The fact that 3 out of the 4 people commenting on this thread have used it at some point provides some evidence that it has more than no value - but on the other hand, there's a cost to keeping it around. The need to guard some sections of code by both #ifdef USE_ASSERT_CHECKING and if (assert_enabled) has occasionally been a source of confusion over the years, and once I had a customer who I was afraid had gotten an assert-enabled build into production and the only way to distinguish between an assert-enabled build with assertions shut off via the GUC and a build that didn't support them in the first place was to try turning the GUC on and see whether it worked, which led to some confusion. Now it looks like we need to add another macro to make this dance even more complicated ... and I'm starting to think it's just not worth it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, May 23, 2014 at 8:15 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> That means you're for a (differently named) disable macro? Or is it not
>> recent enough that you don't care?
> I'm leaning toward thinking we should just rip it out. The fact that
> 3 out of the 4 people commenting on this thread have used it at some
> point provides some evidence that it has more than no value - but on
> the other hand, there's a cost to keeping it around.
Yeah. For the record, I've used it too (don't recall what for exactly).
But I don't think it's worth adding yet another layer of complication for.
The main argument for it given in this thread is recompile cost ...
but TBH, I have one word for anybody who's worried about that, and
that word is "ccache". If you don't have that tool installed, you're
missing out on a huge timesaver.
regards, tom lane
On 2014-05-23 09:56:03 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Fri, May 23, 2014 at 8:15 AM, Andres Freund <andres@2ndquadrant.com> wrote: > >> That means you're for a (differently named) disable macro? Or is it not > >> recent enough that you don't care? > > > I'm leaning toward thinking we should just rip it out. The fact that > > 3 out of the 4 people commenting on this thread have used it at some > > point provides some evidence that it has more than no value - but on > > the other hand, there's a cost to keeping it around. > > Yeah. For the record, I've used it too (don't recall what for exactly). > But I don't think it's worth adding yet another layer of complication for. Cool. Seems like we have an agreement then. The next question is whether to wait till after the branching with this? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> The next question is whether to wait till after the branching with this?
+1 for waiting (it's only a couple weeks anyway). This isn't a
user-facing feature in any way, so I feel no urgency to ship it in 9.4.
regards, tom lane
On 2014-05-23 10:01:37 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > The next question is whether to wait till after the branching with this? > > +1 for waiting (it's only a couple weeks anyway). This isn't a > user-facing feature in any way, so I feel no urgency to ship it in 9.4. Here's my patch for this that I plan to apply later. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On 2014-06-19 19:31:28 +0200, Andres Freund wrote: > On 2014-05-23 10:01:37 -0400, Tom Lane wrote: > > Andres Freund <andres@2ndquadrant.com> writes: > > > The next question is whether to wait till after the branching with this? > > > > +1 for waiting (it's only a couple weeks anyway). This isn't a > > user-facing feature in any way, so I feel no urgency to ship it in 9.4. > > Here's my patch for this that I plan to apply later. Hm, that missed a couple things. Updated patch attached. Besides adding the missed documentation adjustments this also removes the -A commandline/startup packet parameter since it doesn't serve a purpose anymore. Does anyone think we should rather keep -A and have it not to anything? It seems fairly unlikely, but not impossible, that someone had the great idea to add -A off in their connection options. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Andres Freund <andres@2ndquadrant.com> writes:
> Hm, that missed a couple things. Updated patch attached. Besides adding
> the missed documentation adjustments this also removes the -A
> commandline/startup packet parameter since it doesn't serve a purpose
> anymore.
> Does anyone think we should rather keep -A and have it not to anything?
> It seems fairly unlikely, but not impossible, that someone had the
> great idea to add -A off in their connection options.
I think we could delete it. If anyone is using that, I think they'd
be happier getting an error than having it silently not do what they
want (and more slowly than before ...)
regards, tom lane