Обсуждение: Assert for frontend programs?

As I'm working through the parallel dump patch, I notice this in one of 
the header files:

#ifdef USE_ASSERT_CHECKING
#define Assert(condition) \    if (!(condition)) \    { \        write_msg(NULL, "Failed assertion in %s, line %d\n", \
                __FILE__, __LINE__); \        abort();\    }
 
#else
#define Assert(condition)
#endif


I'm wondering if we should have something like this centrally (e.g. in 
postgres_fe.h)? I can certainly see people wanting to be able to use 
Assert in frontend programs generally, and it makes sense to me not to 
make everyone roll their own.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> As I'm working through the parallel dump patch, I notice this in one of 
> the header files:

> #ifdef USE_ASSERT_CHECKING
> #define Assert(condition) \
>      if (!(condition)) \
>      { \
>          write_msg(NULL, "Failed assertion in %s, line %d\n", \
>                    __FILE__, __LINE__); \
>          abort();\
>      }
> #else
> #define Assert(condition)
> #endif


> I'm wondering if we should have something like this centrally (e.g. in 
> postgres_fe.h)? I can certainly see people wanting to be able to use 
> Assert in frontend programs generally, and it makes sense to me not to 
> make everyone roll their own.

+1, especially if the hand-rolled versions are likely to be as bad as
that one (dangling else, maybe some other issues I'm not spotting
in advance of caffeine consumption).  I've wished for frontend Assert
a few times myself, but never bothered to make it happen.

Although I think we had this discussion earlier and it stalled at
figuring out exactly what the "print error" part of the macro ought
to be.  The above is obviously pg_dump-specific.  Perhaps
fprintf(stderr,...) would be sufficient, though -- it's not like
tremendous user friendliness ought to be necessary here.

Also, I think the message really has to include some string-ified
version of the assertion condition --- the line number alone is pretty
unhelpful when looking at field reports of uncertain provenance.

BTW, I think psql already has a "psql_assert".
        regards, tom lane

On 14.12.2012 17:54, Tom Lane wrote:
> Andrew Dunstan<andrew@dunslane.net>  writes:
>> As I'm working through the parallel dump patch, I notice this in one of
>> the header files:
>
>> #ifdef USE_ASSERT_CHECKING
>> #define Assert(condition) \
>>       if (!(condition)) \
>>       { \
>>           write_msg(NULL, "Failed assertion in %s, line %d\n", \
>>                     __FILE__, __LINE__); \
>>           abort();\
>>       }
>> #else
>> #define Assert(condition)
>> #endif
>
>
>> I'm wondering if we should have something like this centrally (e.g. in
>> postgres_fe.h)? I can certainly see people wanting to be able to use
>> Assert in frontend programs generally, and it makes sense to me not to
>> make everyone roll their own.
>
> +1, especially if the hand-rolled versions are likely to be as bad as
> that one (dangling else, maybe some other issues I'm not spotting
> in advance of caffeine consumption).  I've wished for frontend Assert
> a few times myself, but never bothered to make it happen.

+1, I just ran into this while working on Andres' xlogreader patch. 
xlogreader uses Assert(), and it's supposed to work in a stand-alone 
program.

> Although I think we had this discussion earlier and it stalled at
> figuring out exactly what the "print error" part of the macro ought
> to be.  The above is obviously pg_dump-specific.  Perhaps
> fprintf(stderr,...) would be sufficient, though -- it's not like
> tremendous user friendliness ought to be necessary here.
>
> Also, I think the message really has to include some string-ified
> version of the assertion condition --- the line number alone is pretty
> unhelpful when looking at field reports of uncertain provenance.
>
> BTW, I think psql already has a "psql_assert".

psql_assert looks like this:

#ifdef USE_ASSERT_CHECKING
#include <assert.h>
#define psql_assert(p) assert(p)
#else
...

On my Linux system, a failure looks like this:

~$ ./a.out
a.out: a.c:5: main: Assertion `1==2' failed.
Aborted

That seems fine to me.

- Heikki

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 14.12.2012 17:54, Tom Lane wrote:
>> BTW, I think psql already has a "psql_assert".

> psql_assert looks like this:

> #ifdef USE_ASSERT_CHECKING
> #include <assert.h>
> #define psql_assert(p) assert(p)
> #else
> ...

> On my Linux system, a failure looks like this:

> ~$ ./a.out
> a.out: a.c:5: main: Assertion `1==2' failed.
> Aborted

> That seems fine to me.

Works for me.  So just rename that to Assert() and move it into
postgres-fe.h?
        regards, tom lane

On 12/14/2012 11:33 AM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> On 14.12.2012 17:54, Tom Lane wrote:
>>> BTW, I think psql already has a "psql_assert".
>> psql_assert looks like this:
>> #ifdef USE_ASSERT_CHECKING
>> #include <assert.h>
>> #define psql_assert(p) assert(p)
>> #else
>> ...
>> On my Linux system, a failure looks like this:
>> ~$ ./a.out
>> a.out: a.c:5: main: Assertion `1==2' failed.
>> Aborted
>> That seems fine to me.
> Works for me.  So just rename that to Assert() and move it into
> postgres-fe.h?
>
>             


Seems so simple it's a wonder we didn't do it before. +1.

cheers

andrew

On 12/14/2012 11:33 AM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas@vmware.com> writes:
>> On 14.12.2012 17:54, Tom Lane wrote:
>>> BTW, I think psql already has a "psql_assert".
>> psql_assert looks like this:
>> #ifdef USE_ASSERT_CHECKING
>> #include <assert.h>
>> #define psql_assert(p) assert(p)
>> #else
>> ...
>> On my Linux system, a failure looks like this:
>> ~$ ./a.out
>> a.out: a.c:5: main: Assertion `1==2' failed.
>> Aborted
>> That seems fine to me.
> Works for me.  So just rename that to Assert() and move it into
> postgres-fe.h?
>
>


Here's a patch for that. I changed some of the psql assertions so they
all have explicit boolean expressions - I think that's better style for
use of assert.

I noticed, BTW, that there are one or two places in backend code that
seem to call plain assert unconditionally, notably src/port/open.c,
src/backend/utils/adt/inet_net_pton.c and some contrib modules. That
seems undesirable. Should we need to look at turning these into Assert
calls?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> I noticed, BTW, that there are one or two places in backend code that 
> seem to call plain assert unconditionally, notably src/port/open.c, 
> src/backend/utils/adt/inet_net_pton.c and some contrib modules. That 
> seems undesirable. Should we need to look at turning these into Assert 
> calls?

Yeah, possibly.  The inet_net_pton.c case is surely because it was that
way in the BIND code we borrowed; perhaps the others are the same story.
I don't object to changing them, since we don't seem to be actively
adopting any new upstream versions; but again I can't get too excited.

> -            psql_assert(!*text);
> +            Assert(*text != '\0');

I think you got that one backwards.
>  #include "c.h"
> +#ifdef USE_ASSERT_CHECKING
> +#include <assert.h>
> +#define Assert(p) assert(p)
> +#else
> +#define Assert(p)
> +#endif

Perhaps a comment would be in order here?  Specifically something
about providing Assert() so that it can be used in both backend
and frontend code?
        regards, tom lane

On 12/14/12 11:33 AM, Tom Lane wrote:
> Works for me.  So just rename that to Assert() and move it into
> postgres-fe.h?

Or just call assert() and don't invent our own layer?

On 12/14/2012 04:23 PM, Peter Eisentraut wrote:
> On 12/14/12 11:33 AM, Tom Lane wrote:
>> Works for me.  So just rename that to Assert() and move it into
>> postgres-fe.h?
> Or just call assert() and don't invent our own layer?


Well, part of the point is that it lets you use Assert() in code that 
might be run in both the frontend and the backend.

cheers

andrew

Peter Eisentraut <peter_e@gmx.net> writes:
> On 12/14/12 11:33 AM, Tom Lane wrote:
>> Works for me.  So just rename that to Assert() and move it into
>> postgres-fe.h?

> Or just call assert() and don't invent our own layer?

Having the layer is a good thing, eg so that USE_ASSERT_CHECKING
can control it, or so that somebody can inject a different behavior
if they want.
        regards, tom lane

On Fri, 2012-12-14 at 17:03 -0500, Tom Lane wrote:
> Having the layer is a good thing, eg so that USE_ASSERT_CHECKING
> can control it, or so that somebody can inject a different behavior
> if they want. 

You could also (or at least additionally) map !USE_ASSERT_CHECKING to
NDEBUG.  This would also help with imported code that calls assert()
directly.

On Fri, 2012-12-14 at 15:32 -0500, Andrew Dunstan wrote:
> Here's a patch for that.

It appears that your change has caused new compiler warnings:

encnames.c:9:1: warning: "Assert" redefined
In file included from encnames.c:8:
../../../src/include/postgres_fe.h:36:1: warning: this is the location of the previous definition
wchar.c:10:1: warning: "Assert" redefined
In file included from wchar.c:9:
../../../src/include/postgres_fe.h:36:1: warning: this is the location of the previous definition
encnames.c:9:1: warning: "Assert" redefined
In file included from encnames.c:8:
../../../src/include/postgres_fe.h:36:1: warning: this is the location of the previous definition

>  I changed some of the psql assertions so they 
> all have explicit boolean expressions - I think that's better style for 
> use of assert.

I think not, but I probably wrote most of that originally.

On 12/16/2012 01:29 AM, Peter Eisentraut wrote:
> On Fri, 2012-12-14 at 17:03 -0500, Tom Lane wrote:
>> Having the layer is a good thing, eg so that USE_ASSERT_CHECKING
>> can control it, or so that somebody can inject a different behavior
>> if they want.
> You could also (or at least additionally) map !USE_ASSERT_CHECKING to
> NDEBUG.  This would also help with imported code that calls assert()
> directly.


We should probably do that for both frontend and backend code, no? That 
would get rid of potential problems we already have like inet_net_pton.c 
that I noted the other day.

cheers

andrew

cpluspluscheck is having issues with this change:

In file included from /tmp/cpluspluscheck.QEdLaR/test.cpp:3:0:
./src/include/postgres_fe.h:34:0: warning: "Assert" redefined [enabled by default]
In file included from /tmp/cpluspluscheck.QEdLaR/test.cpp:2:0:
src/include/postgres.h:673:0: note: this is the location of the previous definition

This probably rather an issue with how cpluspluscheck is set up, since
including both postgres.h and postgres_fe.h is not useful.