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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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)
#endifI'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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
feassert.patchtext/x-patch; name=feassert.patchDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 8ccd00d..e605785 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -99,7 +99,7 @@ HandleSlashCmds(PsqlScanState scan_state,
char *cmd;
char *arg;
- psql_assert(scan_state);
+ Assert(scan_state != NULL);
/* Parse off the command name */
cmd = psql_scan_slash_command(scan_state);
@@ -1819,7 +1819,7 @@ editFile(const char *fname, int lineno)
char *sys;
int result;
- psql_assert(fname);
+ Assert(fname != NULL);
/* Find an editor to use */
editorName = getenv("PSQL_EDITOR");
@@ -2177,7 +2177,7 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
{
size_t vallen = 0;
- psql_assert(param);
+ Assert(param != NULL);
if (value)
vallen = strlen(value);
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 179c162..c2a2ab6 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1160,7 +1160,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
}
OK = AcceptResult(results);
- psql_assert(!OK);
+ Assert(!OK);
PQclear(results);
break;
}
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index f54baab..7f34290 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -12,13 +12,6 @@
#include <setjmp.h>
#include "libpq-fe.h"
-#ifdef USE_ASSERT_CHECKING
-#include <assert.h>
-#define psql_assert(p) assert(p)
-#else
-#define psql_assert(p)
-#endif
-
#define atooid(x) ((Oid) strtoul((x), NULL, 10))
/*
diff --git a/src/bin/psql/psqlscan.l b/src/bin/psql/psqlscan.l
index 6c14298..f8822cc 100644
--- a/src/bin/psql/psqlscan.l
+++ b/src/bin/psql/psqlscan.l
@@ -1184,8 +1184,8 @@ psql_scan_setup(PsqlScanState state,
const char *line, int line_len)
{
/* Mustn't be scanning already */
- psql_assert(state->scanbufhandle == NULL);
- psql_assert(state->buffer_stack == NULL);
+ Assert(state->scanbufhandle == NULL);
+ Assert(state->buffer_stack == NULL);
/* Do we need to hack the character set encoding? */
state->encoding = pset.encoding;
@@ -1245,7 +1245,7 @@ psql_scan(PsqlScanState state,
int lexresult;
/* Must be scanning already */
- psql_assert(state->scanbufhandle);
+ Assert(state->scanbufhandle != NULL);
/* Set up static variables that will be used by yylex */
cur_state = state;
@@ -1424,7 +1424,7 @@ psql_scan_slash_command(PsqlScanState state)
PQExpBufferData mybuf;
/* Must be scanning already */
- psql_assert(state->scanbufhandle);
+ Assert(state->scanbufhandle != NULL);
/* Build a local buffer that we'll return the data of */
initPQExpBuffer(&mybuf);
@@ -1478,7 +1478,7 @@ psql_scan_slash_option(PsqlScanState state,
char local_quote;
/* Must be scanning already */
- psql_assert(state->scanbufhandle);
+ Assert(state->scanbufhandle != NULL);
if (quote == NULL)
quote = &local_quote;
@@ -1512,7 +1512,7 @@ psql_scan_slash_option(PsqlScanState state,
* or LEXRES_EOL (the latter indicating end of string). If we were inside
* a quoted string, as indicated by YY_START, EOL is an error.
*/
- psql_assert(lexresult == LEXRES_EOL || lexresult == LEXRES_OK);
+ Assert(lexresult == LEXRES_EOL || lexresult == LEXRES_OK);
switch (YY_START)
{
@@ -1608,7 +1608,7 @@ void
psql_scan_slash_command_end(PsqlScanState state)
{
/* Must be scanning already */
- psql_assert(state->scanbufhandle);
+ Assert(state->scanbufhandle != NULL);
/* Set up static variables that will be used by yylex */
cur_state = state;
diff --git a/src/bin/psql/stringutils.c b/src/bin/psql/stringutils.c
index b557c5a..f0bed2b 100644
--- a/src/bin/psql/stringutils.c
+++ b/src/bin/psql/stringutils.c
@@ -245,8 +245,8 @@ strip_quotes(char *source, char quote, char escape, int encoding)
char *src;
char *dst;
- psql_assert(source);
- psql_assert(quote);
+ Assert(source != NULL);
+ Assert(quote != '\0');
src = dst = source;
@@ -299,8 +299,8 @@ quote_if_needed(const char *source, const char *entails_quote,
char *dst;
bool need_quotes = false;
- psql_assert(source);
- psql_assert(quote);
+ Assert(source != NULL);
+ Assert(quote != '\0');
src = source;
dst = ret = pg_malloc(2 * strlen(src) + 3); /* excess */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 18a2595..2241fa3 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3558,7 +3558,7 @@ complete_from_list(const char *text, int state)
const char *item;
/* need to have a list */
- psql_assert(completion_charpp);
+ Assert(completion_charpp != NULL);
/* Initialization */
if (state == 0)
@@ -3620,7 +3620,7 @@ complete_from_list(const char *text, int state)
static char *
complete_from_const(const char *text, int state)
{
- psql_assert(completion_charp);
+ Assert(completion_charp != NULL);
if (state == 0)
{
if (completion_case_sensitive)
@@ -3708,7 +3708,7 @@ complete_from_files(const char *text, int state)
/* expect a NULL return for the empty string only */
if (!unquoted_text)
{
- psql_assert(!*text);
+ Assert(*text != '\0');
unquoted_text = text;
}
}
diff --git a/src/include/postgres_fe.h b/src/include/postgres_fe.h
index fcfbd31..68262a7 100644
--- a/src/include/postgres_fe.h
+++ b/src/include/postgres_fe.h
@@ -24,4 +24,11 @@
#include "c.h"
+#ifdef USE_ASSERT_CHECKING
+#include <assert.h>
+#define Assert(p) assert(p)
+#else
+#define Assert(p)
+#endif
+
#endif /* POSTGRES_FE_H */
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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?
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers