Horton
Menu

Thread

  1. Assert for frontend programs?

    Andrew Dunstan <andrew@dunslane.net> — 2012-12-14T15:27:15Z

    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

  2. Re: Assert for frontend programs?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-14T15:54:35Z

    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

  3. Re: Assert for frontend programs?

    Heikki Linnakangas <hlinnakangas@vmware.com> — 2012-12-14T16:20:03Z

    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

  4. Re: Assert for frontend programs?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-14T16:33:13Z

    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

  5. Re: Assert for frontend programs?

    Andrew Dunstan <andrew@dunslane.net> — 2012-12-14T16:36:01Z

    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

  6. Re: Assert for frontend programs?

    Andrew Dunstan <andrew@dunslane.net> — 2012-12-14T20:32:16Z

    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

  7. Re: Assert for frontend programs?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-14T20:48:29Z

    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

  8. Re: Assert for frontend programs?

    Peter Eisentraut <peter_e@gmx.net> — 2012-12-14T21:23:57Z

    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?

  9. Re: Assert for frontend programs?

    Andrew Dunstan <andrew@dunslane.net> — 2012-12-14T21:28:24Z

    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

  10. Re: Assert for frontend programs?

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-14T22:03:35Z

    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

  11. Re: Assert for frontend programs?

    Peter Eisentraut <peter_e@gmx.net> — 2012-12-16T06:29:47Z

    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.

  12. Re: Assert for frontend programs?

    Peter Eisentraut <peter_e@gmx.net> — 2012-12-16T06:32:24Z

    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.

  13. Re: Assert for frontend programs?

    Andrew Dunstan <andrew@dunslane.net> — 2012-12-16T14:56:01Z

    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

  14. Re: Assert for frontend programs?

    Peter Eisentraut <peter_e@gmx.net> — 2012-12-18T03:37:39Z

    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.