lots of unused variable warnings in assert-free builds
In builds without --enable-cassert (I guess not many developers use
those a lot), there are quite a few unused variable warnings. These
usually hold some intermediate result that the assert checks later. I
see that in some places our code already uses #ifdef
USE_ASSERT_CHECKING, presumably to hide similar issues. But in most
cases using this would significantly butcher the code. I found that
adding __attribute__((unused)) is cleaner. Attached is a patch that
cleans up all the warnings I encountered.
Attachments:
pg-cassert-unused.patchtext/x-patch; charset=UTF-8; name=pg-cassert-unused.patchDownload
diff --git i/src/backend/access/hash/hashovfl.c w/src/backend/access/hash/hashovfl.c
index 130c296..d4329fb 100644
--- i/src/backend/access/hash/hashovfl.c
+++ w/src/backend/access/hash/hashovfl.c
@@ -391,7 +391,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
uint32 ovflbitno;
int32 bitmappage,
bitmapbit;
+#ifdef USE_ASSERT_CHECKING
Bucket bucket;
+#endif
/* Get information from the doomed page */
_hash_checkpage(rel, ovflbuf, LH_OVERFLOW_PAGE);
@@ -400,7 +402,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
ovflopaque = (HashPageOpaque) PageGetSpecialPointer(ovflpage);
nextblkno = ovflopaque->hasho_nextblkno;
prevblkno = ovflopaque->hasho_prevblkno;
+#ifdef USE_ASSERT_CHECKING
bucket = ovflopaque->hasho_bucket;
+#endif
/*
* Zero the page for debugging's sake; then write and release it. (Note:
diff --git i/src/backend/executor/execCurrent.c w/src/backend/executor/execCurrent.c
index b07161f..2a59fc6 100644
--- i/src/backend/executor/execCurrent.c
+++ w/src/backend/executor/execCurrent.c
@@ -151,7 +151,7 @@ execCurrentOf(CurrentOfExpr *cexpr,
{
ScanState *scanstate;
bool lisnull;
- Oid tuple_tableoid;
+ Oid tuple_tableoid __attribute__((unused));
ItemPointer tuple_tid;
/*
diff --git i/src/backend/executor/nodeMaterial.c w/src/backend/executor/nodeMaterial.c
index b320b54..4ab660e 100644
--- i/src/backend/executor/nodeMaterial.c
+++ w/src/backend/executor/nodeMaterial.c
@@ -66,7 +66,7 @@ ExecMaterial(MaterialState *node)
* Allocate a second read pointer to serve as the mark. We know it
* must have index 1, so needn't store that.
*/
- int ptrno;
+ int ptrno __attribute__((unused));
ptrno = tuplestore_alloc_read_pointer(tuplestorestate,
node->eflags);
diff --git i/src/backend/executor/nodeSetOp.c w/src/backend/executor/nodeSetOp.c
index 7fa5730..c6a88a8 100644
--- i/src/backend/executor/nodeSetOp.c
+++ w/src/backend/executor/nodeSetOp.c
@@ -344,7 +344,7 @@ setop_fill_hash_table(SetOpState *setopstate)
SetOp *node = (SetOp *) setopstate->ps.plan;
PlanState *outerPlan;
int firstFlag;
- bool in_first_rel;
+ bool in_first_rel __attribute__((unused));
/*
* get state info from node
diff --git i/src/backend/executor/nodeWorktablescan.c w/src/backend/executor/nodeWorktablescan.c
index e2f3dd4..e72d1cb 100644
--- i/src/backend/executor/nodeWorktablescan.c
+++ w/src/backend/executor/nodeWorktablescan.c
@@ -30,7 +30,7 @@ static TupleTableSlot *
WorkTableScanNext(WorkTableScanState *node)
{
TupleTableSlot *slot;
- EState *estate;
+ EState *estate __attribute__((unused));
Tuplestorestate *tuplestorestate;
/*
diff --git i/src/backend/libpq/be-fsstubs.c w/src/backend/libpq/be-fsstubs.c
index b864c86..1bf765c 100644
--- i/src/backend/libpq/be-fsstubs.c
+++ w/src/backend/libpq/be-fsstubs.c
@@ -378,7 +378,7 @@ lo_import_internal(text *filename, Oid lobjOid)
{
File fd;
int nbytes,
- tmp;
+ tmp __attribute__((unused));
char buf[BUFSIZE];
char fnamebuf[MAXPGPATH];
LargeObjectDesc *lobj;
diff --git i/src/backend/libpq/pqcomm.c w/src/backend/libpq/pqcomm.c
index 35812f4..df41d60 100644
--- i/src/backend/libpq/pqcomm.c
+++ w/src/backend/libpq/pqcomm.c
@@ -1373,7 +1373,7 @@ fail:
void
pq_putmessage_noblock(char msgtype, const char *s, size_t len)
{
- int res;
+ int res __attribute__((unused));
int required;
/*
diff --git i/src/backend/optimizer/path/costsize.c w/src/backend/optimizer/path/costsize.c
index e1c070e..fd16cb1 100644
--- i/src/backend/optimizer/path/costsize.c
+++ w/src/backend/optimizer/path/costsize.c
@@ -3245,7 +3245,7 @@ void
set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
{
PlannerInfo *subroot = rel->subroot;
- RangeTblEntry *rte;
+ RangeTblEntry *rte __attribute__((unused));
ListCell *lc;
/* Should only be applied to base relations that are subqueries */
diff --git i/src/backend/optimizer/plan/createplan.c w/src/backend/optimizer/plan/createplan.c
index e41d2a6..7dccf7d 100644
--- i/src/backend/optimizer/plan/createplan.c
+++ w/src/backend/optimizer/plan/createplan.c
@@ -1820,7 +1820,7 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
ForeignScan *scan_plan;
RelOptInfo *rel = best_path->path.parent;
Index scan_relid = rel->relid;
- RangeTblEntry *rte;
+ RangeTblEntry *rte __attribute__((unused));
bool fsSystemCol;
int i;
diff --git i/src/backend/parser/analyze.c w/src/backend/parser/analyze.c
index be6e93e..d1b9d4e 100644
--- i/src/backend/parser/analyze.c
+++ w/src/backend/parser/analyze.c
@@ -1530,7 +1530,7 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
/* Process leaf SELECT */
Query *selectQuery;
char selectName[32];
- RangeTblEntry *rte;
+ RangeTblEntry *rte __attribute__((unused));
RangeTblRef *rtr;
ListCell *tl;
diff --git i/src/backend/storage/file/fd.c w/src/backend/storage/file/fd.c
index 43bc43a..5021cdc 100644
--- i/src/backend/storage/file/fd.c
+++ w/src/backend/storage/file/fd.c
@@ -684,7 +684,7 @@ LruInsert(File file)
/* seek to the right position */
if (vfdP->seekPos != (off_t) 0)
{
- off_t returnValue;
+ off_t returnValue __attribute__((unused));
returnValue = lseek(vfdP->fd, vfdP->seekPos, SEEK_SET);
Assert(returnValue != (off_t) -1);
diff --git i/src/backend/storage/lmgr/predicate.c w/src/backend/storage/lmgr/predicate.c
index 821328b..839ed9b 100644
--- i/src/backend/storage/lmgr/predicate.c
+++ w/src/backend/storage/lmgr/predicate.c
@@ -2013,7 +2013,7 @@ RestoreScratchTarget(bool lockheld)
static void
RemoveTargetIfNoLongerUsed(PREDICATELOCKTARGET *target, uint32 targettaghash)
{
- PREDICATELOCKTARGET *rmtarget;
+ PREDICATELOCKTARGET *rmtarget __attribute__((unused));
Assert(LWLockHeldByMe(SerializablePredicateLockListLock));
@@ -2074,7 +2074,7 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
{
uint32 oldtargettaghash;
LWLockId partitionLock;
- PREDICATELOCK *rmpredlock;
+ PREDICATELOCK *rmpredlock __attribute__((unused));
oldtargettaghash = PredicateLockTargetTagHashCode(&oldtargettag);
partitionLock = PredicateLockHashPartitionLock(oldtargettaghash);
@@ -2227,7 +2227,7 @@ DecrementParentLocks(const PREDICATELOCKTARGETTAG *targettag)
{
uint32 targettaghash;
LOCALPREDICATELOCK *parentlock,
- *rmlock;
+ *rmlock __attribute__((unused));
parenttag = nexttag;
targettaghash = PredicateLockTargetTagHashCode(&parenttag);
diff --git i/src/backend/storage/lmgr/proc.c w/src/backend/storage/lmgr/proc.c
index dcf1928..6927fe6 100644
--- i/src/backend/storage/lmgr/proc.c
+++ w/src/backend/storage/lmgr/proc.c
@@ -807,7 +807,7 @@ static void
AuxiliaryProcKill(int code, Datum arg)
{
int proctype = DatumGetInt32(arg);
- PGPROC *auxproc;
+ PGPROC *auxproc __attribute__((unused));
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
diff --git i/src/backend/utils/adt/selfuncs.c w/src/backend/utils/adt/selfuncs.c
index da638f8..cc11363 100644
--- i/src/backend/utils/adt/selfuncs.c
+++ w/src/backend/utils/adt/selfuncs.c
@@ -3771,7 +3771,7 @@ convert_string_datum(Datum value, Oid typid)
{
char *xfrmstr;
size_t xfrmlen;
- size_t xfrmlen2;
+ size_t xfrmlen2 __attribute__((unused));
/*
* Note: originally we guessed at a suitable output buffer size, and
@@ -6281,7 +6281,7 @@ btcostestimate(PG_FUNCTION_ARGS)
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcc);
Expr *clause;
Node *leftop,
- *rightop;
+ *rightop __attribute__((unused));
Oid clause_op;
int op_strategy;
bool is_null_op = false;
diff --git i/src/bin/psql/psqlscan.l w/src/bin/psql/psqlscan.l
index a27bcd1..0f69f54 100644
--- i/src/bin/psql/psqlscan.l
+++ w/src/bin/psql/psqlscan.l
@@ -1474,7 +1474,7 @@ psql_scan_slash_option(PsqlScanState state,
bool semicolon)
{
PQExpBufferData mybuf;
- int lexresult;
+ int lexresult __attribute__((unused));
char local_quote;
/* Must be scanning already */
Peter Eisentraut <peter_e@gmx.net> writes:
I see that in some places our code already uses #ifdef
USE_ASSERT_CHECKING, presumably to hide similar issues. But in most
cases using this would significantly butcher the code. I found that
adding __attribute__((unused)) is cleaner. Attached is a patch that
cleans up all the warnings I encountered.
Surely this will fail entirely on most non-gcc compilers? Not to
mention that next month's gcc may complain "hey, you used this 'unused'
variable". I think #ifdef USE_ASSERT_CHECKING is really the only way
if you care about quieting these warnings. (Personally, I don't.)
regards, tom lane
On 01/15/2012 01:37 AM, Tom Lane wrote:
Peter Eisentraut<peter_e@gmx.net> writes:
I see that in some places our code already uses #ifdef
USE_ASSERT_CHECKING, presumably to hide similar issues. But in most
cases using this would significantly butcher the code. I found that
adding __attribute__((unused)) is cleaner. Attached is a patch that
cleans up all the warnings I encountered.Surely this will fail entirely on most non-gcc compilers? Not to
mention that next month's gcc may complain "hey, you used this 'unused'
variable". I think #ifdef USE_ASSERT_CHECKING is really the only way
if you care about quieting these warnings. (Personally, I don't.)
It would possibly have some documentary value too. Just looking very
quickly at Peter's patch, I don't really understand his assertion that
this would significantly butcher the code. The worst effect would be
that in a few cases we'd have to break up multiple declarations where
one of the variables was in this class. That doesn't seem like a tragedy.
I like software that compiles in the normal use with few or no warnings.
I should have thought that would appeal to most packagers, too.
cheers
andrew
On Sun, Jan 15, 2012 at 1:14 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 01/15/2012 01:37 AM, Tom Lane wrote:
Peter Eisentraut<peter_e@gmx.net> writes:
I see that in some places our code already uses #ifdef
USE_ASSERT_CHECKING, presumably to hide similar issues. But in most
cases using this would significantly butcher the code. I found that
adding __attribute__((unused)) is cleaner. Attached is a patch that
cleans up all the warnings I encountered.Surely this will fail entirely on most non-gcc compilers? Not to
mention that next month's gcc may complain "hey, you used this 'unused'
variable". I think #ifdef USE_ASSERT_CHECKING is really the only way
if you care about quieting these warnings. (Personally, I don't.)It would possibly have some documentary value too. Just looking very quickly
at Peter's patch, I don't really understand his assertion that this would
significantly butcher the code. The worst effect would be that in a few
cases we'd have to break up multiple declarations where one of the variables
was in this class. That doesn't seem like a tragedy.I like software that compiles in the normal use with few or no warnings. I
should have thought that would appeal to most packagers, too.
Sounds good, but let's not do it yet because we have a few patches to
commit first.
It would be good to minimise bit rot during the CF.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On sön, 2012-01-15 at 01:37 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
I see that in some places our code already uses #ifdef
USE_ASSERT_CHECKING, presumably to hide similar issues. But in most
cases using this would significantly butcher the code. I found that
adding __attribute__((unused)) is cleaner. Attached is a patch that
cleans up all the warnings I encountered.Surely this will fail entirely on most non-gcc compilers?
No, because __attribute__() is defined to empty for other compilers. We
use this pattern already.
Not to
mention that next month's gcc may complain "hey, you used this 'unused'
variable".
No, because __attribute__((unused)) means "that the variable is meant to
be possibly unused".
On sön, 2012-01-15 at 08:14 -0500, Andrew Dunstan wrote:
It would possibly have some documentary value too. Just looking very
quickly at Peter's patch, I don't really understand his assertion that
this would significantly butcher the code. The worst effect would be
that in a few cases we'd have to break up multiple declarations where
one of the variables was in this class. That doesn't seem like a
tragedy.
Well, I'll prepare a patch like that and then you can judge.
Peter Eisentraut <peter_e@gmx.net> writes:
On sön, 2012-01-15 at 01:37 -0500, Tom Lane wrote:
Surely this will fail entirely on most non-gcc compilers?
No, because __attribute__() is defined to empty for other compilers. We
use this pattern already.
Oh, duh. In that case my only objection to doing it like this is that
I'd like to see what pgindent will do with it. pgindent is not very
nice about #ifdef's in variable lists (it tends to insert unwanted
vertical space) so it's possible that this way will look better.
regards, tom lane
On ons, 2012-01-18 at 21:16 +0200, Peter Eisentraut wrote:
On sön, 2012-01-15 at 08:14 -0500, Andrew Dunstan wrote:
It would possibly have some documentary value too. Just looking very
quickly at Peter's patch, I don't really understand his assertion that
this would significantly butcher the code. The worst effect would be
that in a few cases we'd have to break up multiple declarations where
one of the variables was in this class. That doesn't seem like a
tragedy.Well, I'll prepare a patch like that and then you can judge.
So, here are three patches that could solve this issue.
pg-cassert-unused-attribute.patch, the one I already showed, using
__attribute__((unused).
pg-cassert-unused-ifdef.patch, using only additional #ifdef
USE_ASSERT_CHECKING. This does have additional documentation value, but
you can see that it gets bulky and complicated. (I introduced several
bugs merely while creating this patch.)
pg-cassert-unused-void.patch is an alternative approach that avoids the
warnings by casting the arguments of Assert() to void if assertions are
disabled. This has the least code impact, but it changes the
traditional semantics of asserts, which is that they disappear
completely when not enabled. You can see how this creates a problem in
src/backend/replication/syncrep.c, where the nontrivial call to
SyncRepQueueIsOrderedByLSN() now becomes visible even in non-assert
builds. I played around with some other options like
__attribute__((pure)) to make the compiler optimize the function away in
that case, but that didn't appear to work. So this might not be a
workable solution (and it would be GCC-specific anyway).
Attachments:
pg-cassert-unused-attribute.patchtext/x-patch; charset=UTF-8; name=pg-cassert-unused-attribute.patchDownload
diff --git i/src/backend/access/hash/hashovfl.c w/src/backend/access/hash/hashovfl.c
index 130c296..d4329fb 100644
--- i/src/backend/access/hash/hashovfl.c
+++ w/src/backend/access/hash/hashovfl.c
@@ -391,7 +391,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
uint32 ovflbitno;
int32 bitmappage,
bitmapbit;
+#ifdef USE_ASSERT_CHECKING
Bucket bucket;
+#endif
/* Get information from the doomed page */
_hash_checkpage(rel, ovflbuf, LH_OVERFLOW_PAGE);
@@ -400,7 +402,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
ovflopaque = (HashPageOpaque) PageGetSpecialPointer(ovflpage);
nextblkno = ovflopaque->hasho_nextblkno;
prevblkno = ovflopaque->hasho_prevblkno;
+#ifdef USE_ASSERT_CHECKING
bucket = ovflopaque->hasho_bucket;
+#endif
/*
* Zero the page for debugging's sake; then write and release it. (Note:
diff --git i/src/backend/executor/execCurrent.c w/src/backend/executor/execCurrent.c
index b07161f..2a59fc6 100644
--- i/src/backend/executor/execCurrent.c
+++ w/src/backend/executor/execCurrent.c
@@ -151,7 +151,7 @@ execCurrentOf(CurrentOfExpr *cexpr,
{
ScanState *scanstate;
bool lisnull;
- Oid tuple_tableoid;
+ Oid tuple_tableoid __attribute__((unused));
ItemPointer tuple_tid;
/*
diff --git i/src/backend/executor/nodeMaterial.c w/src/backend/executor/nodeMaterial.c
index b320b54..4ab660e 100644
--- i/src/backend/executor/nodeMaterial.c
+++ w/src/backend/executor/nodeMaterial.c
@@ -66,7 +66,7 @@ ExecMaterial(MaterialState *node)
* Allocate a second read pointer to serve as the mark. We know it
* must have index 1, so needn't store that.
*/
- int ptrno;
+ int ptrno __attribute__((unused));
ptrno = tuplestore_alloc_read_pointer(tuplestorestate,
node->eflags);
diff --git i/src/backend/executor/nodeSetOp.c w/src/backend/executor/nodeSetOp.c
index 7fa5730..c6a88a8 100644
--- i/src/backend/executor/nodeSetOp.c
+++ w/src/backend/executor/nodeSetOp.c
@@ -344,7 +344,7 @@ setop_fill_hash_table(SetOpState *setopstate)
SetOp *node = (SetOp *) setopstate->ps.plan;
PlanState *outerPlan;
int firstFlag;
- bool in_first_rel;
+ bool in_first_rel __attribute__((unused));
/*
* get state info from node
diff --git i/src/backend/executor/nodeWorktablescan.c w/src/backend/executor/nodeWorktablescan.c
index e2f3dd4..e72d1cb 100644
--- i/src/backend/executor/nodeWorktablescan.c
+++ w/src/backend/executor/nodeWorktablescan.c
@@ -30,7 +30,7 @@ static TupleTableSlot *
WorkTableScanNext(WorkTableScanState *node)
{
TupleTableSlot *slot;
- EState *estate;
+ EState *estate __attribute__((unused));
Tuplestorestate *tuplestorestate;
/*
diff --git i/src/backend/libpq/be-fsstubs.c w/src/backend/libpq/be-fsstubs.c
index b864c86..1bf765c 100644
--- i/src/backend/libpq/be-fsstubs.c
+++ w/src/backend/libpq/be-fsstubs.c
@@ -378,7 +378,7 @@ lo_import_internal(text *filename, Oid lobjOid)
{
File fd;
int nbytes,
- tmp;
+ tmp __attribute__((unused));
char buf[BUFSIZE];
char fnamebuf[MAXPGPATH];
LargeObjectDesc *lobj;
diff --git i/src/backend/libpq/pqcomm.c w/src/backend/libpq/pqcomm.c
index 35812f4..df41d60 100644
--- i/src/backend/libpq/pqcomm.c
+++ w/src/backend/libpq/pqcomm.c
@@ -1373,7 +1373,7 @@ fail:
void
pq_putmessage_noblock(char msgtype, const char *s, size_t len)
{
- int res;
+ int res __attribute__((unused));
int required;
/*
diff --git i/src/backend/optimizer/path/costsize.c w/src/backend/optimizer/path/costsize.c
index e1c070e..fd16cb1 100644
--- i/src/backend/optimizer/path/costsize.c
+++ w/src/backend/optimizer/path/costsize.c
@@ -3245,7 +3245,7 @@ void
set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
{
PlannerInfo *subroot = rel->subroot;
- RangeTblEntry *rte;
+ RangeTblEntry *rte __attribute__((unused));
ListCell *lc;
/* Should only be applied to base relations that are subqueries */
diff --git i/src/backend/optimizer/plan/createplan.c w/src/backend/optimizer/plan/createplan.c
index e41d2a6..7dccf7d 100644
--- i/src/backend/optimizer/plan/createplan.c
+++ w/src/backend/optimizer/plan/createplan.c
@@ -1820,7 +1820,7 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
ForeignScan *scan_plan;
RelOptInfo *rel = best_path->path.parent;
Index scan_relid = rel->relid;
- RangeTblEntry *rte;
+ RangeTblEntry *rte __attribute__((unused));
bool fsSystemCol;
int i;
diff --git i/src/backend/parser/analyze.c w/src/backend/parser/analyze.c
index be6e93e..d1b9d4e 100644
--- i/src/backend/parser/analyze.c
+++ w/src/backend/parser/analyze.c
@@ -1530,7 +1530,7 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
/* Process leaf SELECT */
Query *selectQuery;
char selectName[32];
- RangeTblEntry *rte;
+ RangeTblEntry *rte __attribute__((unused));
RangeTblRef *rtr;
ListCell *tl;
diff --git i/src/backend/storage/file/fd.c w/src/backend/storage/file/fd.c
index 43bc43a..5021cdc 100644
--- i/src/backend/storage/file/fd.c
+++ w/src/backend/storage/file/fd.c
@@ -684,7 +684,7 @@ LruInsert(File file)
/* seek to the right position */
if (vfdP->seekPos != (off_t) 0)
{
- off_t returnValue;
+ off_t returnValue __attribute__((unused));
returnValue = lseek(vfdP->fd, vfdP->seekPos, SEEK_SET);
Assert(returnValue != (off_t) -1);
diff --git i/src/backend/storage/lmgr/predicate.c w/src/backend/storage/lmgr/predicate.c
index 821328b..839ed9b 100644
--- i/src/backend/storage/lmgr/predicate.c
+++ w/src/backend/storage/lmgr/predicate.c
@@ -2013,7 +2013,7 @@ RestoreScratchTarget(bool lockheld)
static void
RemoveTargetIfNoLongerUsed(PREDICATELOCKTARGET *target, uint32 targettaghash)
{
- PREDICATELOCKTARGET *rmtarget;
+ PREDICATELOCKTARGET *rmtarget __attribute__((unused));
Assert(LWLockHeldByMe(SerializablePredicateLockListLock));
@@ -2074,7 +2074,7 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
{
uint32 oldtargettaghash;
LWLockId partitionLock;
- PREDICATELOCK *rmpredlock;
+ PREDICATELOCK *rmpredlock __attribute__((unused));
oldtargettaghash = PredicateLockTargetTagHashCode(&oldtargettag);
partitionLock = PredicateLockHashPartitionLock(oldtargettaghash);
@@ -2227,7 +2227,7 @@ DecrementParentLocks(const PREDICATELOCKTARGETTAG *targettag)
{
uint32 targettaghash;
LOCALPREDICATELOCK *parentlock,
- *rmlock;
+ *rmlock __attribute__((unused));
parenttag = nexttag;
targettaghash = PredicateLockTargetTagHashCode(&parenttag);
diff --git i/src/backend/storage/lmgr/proc.c w/src/backend/storage/lmgr/proc.c
index dcf1928..6927fe6 100644
--- i/src/backend/storage/lmgr/proc.c
+++ w/src/backend/storage/lmgr/proc.c
@@ -807,7 +807,7 @@ static void
AuxiliaryProcKill(int code, Datum arg)
{
int proctype = DatumGetInt32(arg);
- PGPROC *auxproc;
+ PGPROC *auxproc __attribute__((unused));
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
diff --git i/src/backend/utils/adt/selfuncs.c w/src/backend/utils/adt/selfuncs.c
index da638f8..cc11363 100644
--- i/src/backend/utils/adt/selfuncs.c
+++ w/src/backend/utils/adt/selfuncs.c
@@ -3771,7 +3771,7 @@ convert_string_datum(Datum value, Oid typid)
{
char *xfrmstr;
size_t xfrmlen;
- size_t xfrmlen2;
+ size_t xfrmlen2 __attribute__((unused));
/*
* Note: originally we guessed at a suitable output buffer size, and
@@ -6281,7 +6281,7 @@ btcostestimate(PG_FUNCTION_ARGS)
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcc);
Expr *clause;
Node *leftop,
- *rightop;
+ *rightop __attribute__((unused));
Oid clause_op;
int op_strategy;
bool is_null_op = false;
diff --git i/src/bin/psql/psqlscan.l w/src/bin/psql/psqlscan.l
index a27bcd1..0f69f54 100644
--- i/src/bin/psql/psqlscan.l
+++ w/src/bin/psql/psqlscan.l
@@ -1474,7 +1474,7 @@ psql_scan_slash_option(PsqlScanState state,
bool semicolon)
{
PQExpBufferData mybuf;
- int lexresult;
+ int lexresult __attribute__((unused));
char local_quote;
/* Must be scanning already */
pg-cassert-unused-ifdef.patchtext/x-patch; charset=UTF-8; name=pg-cassert-unused-ifdef.patchDownload
diff --git i/src/backend/access/hash/hashovfl.c w/src/backend/access/hash/hashovfl.c
index 130c296..d4329fb 100644
--- i/src/backend/access/hash/hashovfl.c
+++ w/src/backend/access/hash/hashovfl.c
@@ -391,7 +391,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
uint32 ovflbitno;
int32 bitmappage,
bitmapbit;
+#ifdef USE_ASSERT_CHECKING
Bucket bucket;
+#endif
/* Get information from the doomed page */
_hash_checkpage(rel, ovflbuf, LH_OVERFLOW_PAGE);
@@ -400,7 +402,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
ovflopaque = (HashPageOpaque) PageGetSpecialPointer(ovflpage);
nextblkno = ovflopaque->hasho_nextblkno;
prevblkno = ovflopaque->hasho_prevblkno;
+#ifdef USE_ASSERT_CHECKING
bucket = ovflopaque->hasho_bucket;
+#endif
/*
* Zero the page for debugging's sake; then write and release it. (Note:
diff --git i/src/backend/executor/execCurrent.c w/src/backend/executor/execCurrent.c
index b07161f..41cdfcf 100644
--- i/src/backend/executor/execCurrent.c
+++ w/src/backend/executor/execCurrent.c
@@ -151,7 +151,9 @@ execCurrentOf(CurrentOfExpr *cexpr,
{
ScanState *scanstate;
bool lisnull;
+#ifdef USE_ASSERT_CHECKING
Oid tuple_tableoid;
+#endif
ItemPointer tuple_tid;
/*
@@ -184,7 +186,9 @@ execCurrentOf(CurrentOfExpr *cexpr,
return false;
/* Use slot_getattr to catch any possible mistakes */
+#ifdef USE_ASSERT_CHECKING
tuple_tableoid =
+#endif
DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
TableOidAttributeNumber,
&lisnull));
diff --git i/src/backend/executor/nodeMaterial.c w/src/backend/executor/nodeMaterial.c
index b320b54..6dbfe55 100644
--- i/src/backend/executor/nodeMaterial.c
+++ w/src/backend/executor/nodeMaterial.c
@@ -62,13 +62,16 @@ ExecMaterial(MaterialState *node)
tuplestore_set_eflags(tuplestorestate, node->eflags);
if (node->eflags & EXEC_FLAG_MARK)
{
+#ifdef USE_ASSERT_CHECKING
/*
* Allocate a second read pointer to serve as the mark. We know it
* must have index 1, so needn't store that.
*/
int ptrno;
- ptrno = tuplestore_alloc_read_pointer(tuplestorestate,
+ ptrno =
+#endif
+ tuplestore_alloc_read_pointer(tuplestorestate,
node->eflags);
Assert(ptrno == 1);
}
diff --git i/src/backend/executor/nodeSetOp.c w/src/backend/executor/nodeSetOp.c
index 7fa5730..ee92125 100644
--- i/src/backend/executor/nodeSetOp.c
+++ w/src/backend/executor/nodeSetOp.c
@@ -344,7 +344,9 @@ setop_fill_hash_table(SetOpState *setopstate)
SetOp *node = (SetOp *) setopstate->ps.plan;
PlanState *outerPlan;
int firstFlag;
+#ifdef USE_ASSERT_CHECKING
bool in_first_rel;
+#endif
/*
* get state info from node
@@ -361,7 +363,9 @@ setop_fill_hash_table(SetOpState *setopstate)
* Process each outer-plan tuple, and then fetch the next one, until we
* exhaust the outer plan.
*/
+#ifdef USE_ASSERT_CHECKING
in_first_rel = true;
+#endif
for (;;)
{
TupleTableSlot *outerslot;
@@ -395,7 +399,9 @@ setop_fill_hash_table(SetOpState *setopstate)
else
{
/* reached second relation */
+#ifdef USE_ASSERT_CHECKING
in_first_rel = false;
+#endif
/* For tuples not seen previously, do not make hashtable entry */
entry = (SetOpHashEntry)
diff --git i/src/backend/executor/nodeWorktablescan.c w/src/backend/executor/nodeWorktablescan.c
index e2f3dd4..203fa6c 100644
--- i/src/backend/executor/nodeWorktablescan.c
+++ w/src/backend/executor/nodeWorktablescan.c
@@ -30,7 +30,9 @@ static TupleTableSlot *
WorkTableScanNext(WorkTableScanState *node)
{
TupleTableSlot *slot;
+#ifdef USE_ASSERT_CHECKING
EState *estate;
+#endif
Tuplestorestate *tuplestorestate;
/*
@@ -48,7 +50,9 @@ WorkTableScanNext(WorkTableScanState *node)
* worktable. Therefore, we don't need a private read pointer for the
* tuplestore, nor do we need to tell tuplestore_gettupleslot to copy.
*/
+#ifdef USE_ASSERT_CHECKING
estate = node->ss.ps.state;
+#endif
Assert(ScanDirectionIsForward(estate->es_direction));
tuplestorestate = node->rustate->working_table;
diff --git i/src/backend/libpq/be-fsstubs.c w/src/backend/libpq/be-fsstubs.c
index b864c86..a3c8ad4 100644
--- i/src/backend/libpq/be-fsstubs.c
+++ w/src/backend/libpq/be-fsstubs.c
@@ -377,8 +377,10 @@ static Oid
lo_import_internal(text *filename, Oid lobjOid)
{
File fd;
- int nbytes,
- tmp;
+ int nbytes;
+#ifdef USE_ASSERT_CHECKING
+ int tmp;
+#endif
char buf[BUFSIZE];
char fnamebuf[MAXPGPATH];
LargeObjectDesc *lobj;
@@ -417,7 +419,10 @@ lo_import_internal(text *filename, Oid lobjOid)
while ((nbytes = FileRead(fd, buf, BUFSIZE)) > 0)
{
- tmp = inv_write(lobj, buf, nbytes);
+#ifdef USE_ASSERT_CHECKING
+ tmp =
+#endif
+ inv_write(lobj, buf, nbytes);
Assert(tmp == nbytes);
}
diff --git i/src/backend/libpq/pqcomm.c w/src/backend/libpq/pqcomm.c
index 35812f4..0c256b8 100644
--- i/src/backend/libpq/pqcomm.c
+++ w/src/backend/libpq/pqcomm.c
@@ -1373,7 +1373,9 @@ fail:
void
pq_putmessage_noblock(char msgtype, const char *s, size_t len)
{
+#ifdef USE_ASSERT_CHECKING
int res;
+#endif
int required;
/*
@@ -1386,7 +1388,10 @@ pq_putmessage_noblock(char msgtype, const char *s, size_t len)
PqSendBuffer = repalloc(PqSendBuffer, required);
PqSendBufferSize = required;
}
- res = pq_putmessage(msgtype, s, len);
+#ifdef USE_ASSERT_CHECKING
+ res =
+#endif
+ pq_putmessage(msgtype, s, len);
Assert(res == 0); /* should not fail when the message fits in
* buffer */
}
diff --git i/src/backend/optimizer/path/costsize.c w/src/backend/optimizer/path/costsize.c
index e1c070e..2b9c82a 100644
--- i/src/backend/optimizer/path/costsize.c
+++ w/src/backend/optimizer/path/costsize.c
@@ -3245,12 +3245,16 @@ void
set_subquery_size_estimates(PlannerInfo *root, RelOptInfo *rel)
{
PlannerInfo *subroot = rel->subroot;
+#ifdef USE_ASSERT_CHECKING
RangeTblEntry *rte;
+#endif
ListCell *lc;
/* Should only be applied to base relations that are subqueries */
Assert(rel->relid > 0);
+#ifdef USE_ASSERT_CHECKING
rte = planner_rt_fetch(rel->relid, root);
+#endif
Assert(rte->rtekind == RTE_SUBQUERY);
/* Copy raw number of output rows from subplan */
diff --git i/src/backend/optimizer/plan/createplan.c w/src/backend/optimizer/plan/createplan.c
index e41d2a6..369490d 100644
--- i/src/backend/optimizer/plan/createplan.c
+++ w/src/backend/optimizer/plan/createplan.c
@@ -1820,14 +1820,18 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
ForeignScan *scan_plan;
RelOptInfo *rel = best_path->path.parent;
Index scan_relid = rel->relid;
+#ifdef USE_ASSERT_CHECKING
RangeTblEntry *rte;
+#endif
bool fsSystemCol;
int i;
/* it should be a base rel... */
Assert(scan_relid > 0);
Assert(rel->rtekind == RTE_RELATION);
+#ifdef USE_ASSERT_CHECKING
rte = planner_rt_fetch(scan_relid, root);
+#endif
Assert(rte->rtekind == RTE_RELATION);
/* Sort clauses into best execution order */
diff --git i/src/backend/parser/analyze.c w/src/backend/parser/analyze.c
index be6e93e..b6cf8c2 100644
--- i/src/backend/parser/analyze.c
+++ w/src/backend/parser/analyze.c
@@ -1530,7 +1530,9 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
/* Process leaf SELECT */
Query *selectQuery;
char selectName[32];
+#ifdef USE_ASSERT_CHECKING
RangeTblEntry *rte;
+#endif
RangeTblRef *rtr;
ListCell *tl;
@@ -1579,7 +1581,10 @@ transformSetOperationTree(ParseState *pstate, SelectStmt *stmt,
*/
snprintf(selectName, sizeof(selectName), "*SELECT* %d",
list_length(pstate->p_rtable) + 1);
- rte = addRangeTableEntryForSubquery(pstate,
+#ifdef USE_ASSERT_CHECKING
+ rte =
+#endif
+ addRangeTableEntryForSubquery(pstate,
selectQuery,
makeAlias(selectName, NIL),
false);
diff --git i/src/backend/storage/file/fd.c w/src/backend/storage/file/fd.c
index 43bc43a..915511e 100644
--- i/src/backend/storage/file/fd.c
+++ w/src/backend/storage/file/fd.c
@@ -684,9 +684,12 @@ LruInsert(File file)
/* seek to the right position */
if (vfdP->seekPos != (off_t) 0)
{
+#ifdef USE_ASSERT_CHECKING
off_t returnValue;
- returnValue = lseek(vfdP->fd, vfdP->seekPos, SEEK_SET);
+ returnValue =
+#endif
+ lseek(vfdP->fd, vfdP->seekPos, SEEK_SET);
Assert(returnValue != (off_t) -1);
}
}
diff --git i/src/backend/storage/lmgr/predicate.c w/src/backend/storage/lmgr/predicate.c
index 9e927f8..9134d3e 100644
--- i/src/backend/storage/lmgr/predicate.c
+++ w/src/backend/storage/lmgr/predicate.c
@@ -2013,7 +2013,9 @@ RestoreScratchTarget(bool lockheld)
static void
RemoveTargetIfNoLongerUsed(PREDICATELOCKTARGET *target, uint32 targettaghash)
{
+#ifdef USE_ASSERT_CHECKING
PREDICATELOCKTARGET *rmtarget;
+#endif
Assert(LWLockHeldByMe(SerializablePredicateLockListLock));
@@ -2022,7 +2024,10 @@ RemoveTargetIfNoLongerUsed(PREDICATELOCKTARGET *target, uint32 targettaghash)
return;
/* Actually remove the target. */
- rmtarget = hash_search_with_hash_value(PredicateLockTargetHash,
+#ifdef USE_ASSERT_CHECKING
+ rmtarget =
+#endif
+ hash_search_with_hash_value(PredicateLockTargetHash,
&target->tag,
targettaghash,
HASH_REMOVE, NULL);
@@ -2074,7 +2079,9 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
{
uint32 oldtargettaghash;
LWLockId partitionLock;
+#ifdef USE_ASSERT_CHECKING
PREDICATELOCK *rmpredlock;
+#endif
oldtargettaghash = PredicateLockTargetTagHashCode(&oldtargettag);
partitionLock = PredicateLockHashPartitionLock(oldtargettaghash);
@@ -2083,7 +2090,10 @@ DeleteChildTargetLocks(const PREDICATELOCKTARGETTAG *newtargettag)
SHMQueueDelete(predlocksxactlink);
SHMQueueDelete(&(predlock->targetLink));
- rmpredlock = hash_search_with_hash_value
+#ifdef USE_ASSERT_CHECKING
+ rmpredlock =
+#endif
+ hash_search_with_hash_value
(PredicateLockHash,
&oldlocktag,
PredicateLockHashCodeFromTargetHashCode(&oldlocktag,
@@ -2226,8 +2236,10 @@ DecrementParentLocks(const PREDICATELOCKTARGETTAG *targettag)
while (GetParentPredicateLockTag(&parenttag, &nexttag))
{
uint32 targettaghash;
- LOCALPREDICATELOCK *parentlock,
- *rmlock;
+ LOCALPREDICATELOCK *parentlock;
+#ifdef USE_ASSERT_CHECKING
+ LOCALPREDICATELOCK *rmlock;
+#endif
parenttag = nexttag;
targettaghash = PredicateLockTargetTagHashCode(&parenttag);
@@ -2259,7 +2271,9 @@ DecrementParentLocks(const PREDICATELOCKTARGETTAG *targettag)
if ((parentlock->childLocks == 0) && (!parentlock->held))
{
+#ifdef USE_ASSERT_CHECKING
rmlock = (LOCALPREDICATELOCK *)
+#endif
hash_search_with_hash_value(LocalPredicateLockHash,
&parenttag, targettaghash,
HASH_REMOVE, NULL);
diff --git i/src/backend/storage/lmgr/proc.c w/src/backend/storage/lmgr/proc.c
index dcf1928..2c3a72b 100644
--- i/src/backend/storage/lmgr/proc.c
+++ w/src/backend/storage/lmgr/proc.c
@@ -806,12 +806,16 @@ ProcKill(int code, Datum arg)
static void
AuxiliaryProcKill(int code, Datum arg)
{
+#ifdef USE_ASSERT_CHECKING
int proctype = DatumGetInt32(arg);
PGPROC *auxproc;
+#endif
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+#ifdef USE_ASSERT_CHECKING
auxproc = &AuxiliaryProcs[proctype];
+#endif
Assert(MyProc == auxproc);
diff --git i/src/backend/utils/adt/selfuncs.c w/src/backend/utils/adt/selfuncs.c
index da638f8..a133856 100644
--- i/src/backend/utils/adt/selfuncs.c
+++ w/src/backend/utils/adt/selfuncs.c
@@ -3771,7 +3771,9 @@ convert_string_datum(Datum value, Oid typid)
{
char *xfrmstr;
size_t xfrmlen;
+#ifdef USE_ASSERT_CHECKING
size_t xfrmlen2;
+#endif
/*
* Note: originally we guessed at a suitable output buffer size, and
@@ -3815,7 +3817,10 @@ convert_string_datum(Datum value, Oid typid)
return val;
#endif
xfrmstr = (char *) palloc(xfrmlen + 1);
- xfrmlen2 = strxfrm(xfrmstr, val, xfrmlen + 1);
+#ifdef USE_ASSERT_CHECKING
+ xfrmlen2 =
+#endif
+ strxfrm(xfrmstr, val, xfrmlen + 1);
Assert(xfrmlen2 <= xfrmlen);
pfree(val);
val = xfrmstr;
@@ -6280,8 +6285,10 @@ btcostestimate(PG_FUNCTION_ARGS)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcc);
Expr *clause;
- Node *leftop,
- *rightop;
+ Node *leftop;
+#ifdef USE_ASSERT_CHECKING
+ Node *rightop;
+#endif
Oid clause_op;
int op_strategy;
bool is_null_op = false;
@@ -6303,7 +6310,9 @@ btcostestimate(PG_FUNCTION_ARGS)
if (IsA(clause, OpExpr))
{
leftop = get_leftop(clause);
+#ifdef USE_ASSERT_CHECKING
rightop = get_rightop(clause);
+#endif
clause_op = ((OpExpr *) clause)->opno;
}
else if (IsA(clause, RowCompareExpr))
@@ -6311,7 +6320,9 @@ btcostestimate(PG_FUNCTION_ARGS)
RowCompareExpr *rc = (RowCompareExpr *) clause;
leftop = (Node *) linitial(rc->largs);
+#ifdef USE_ASSERT_CHECKING
rightop = (Node *) linitial(rc->rargs);
+#endif
clause_op = linitial_oid(rc->opnos);
}
else if (IsA(clause, ScalarArrayOpExpr))
@@ -6319,7 +6330,9 @@ btcostestimate(PG_FUNCTION_ARGS)
ScalarArrayOpExpr *saop = (ScalarArrayOpExpr *) clause;
leftop = (Node *) linitial(saop->args);
+#ifdef USE_ASSERT_CHECKING
rightop = (Node *) lsecond(saop->args);
+#endif
clause_op = saop->opno;
found_saop = true;
}
@@ -6328,7 +6341,9 @@ btcostestimate(PG_FUNCTION_ARGS)
NullTest *nt = (NullTest *) clause;
leftop = (Node *) nt->arg;
+#ifdef USE_ASSERT_CHECKING
rightop = NULL;
+#endif
clause_op = InvalidOid;
if (nt->nulltesttype == IS_NULL)
{
diff --git i/src/bin/psql/psqlscan.l w/src/bin/psql/psqlscan.l
index a27bcd1..545ab02 100644
--- i/src/bin/psql/psqlscan.l
+++ w/src/bin/psql/psqlscan.l
@@ -1474,7 +1474,9 @@ psql_scan_slash_option(PsqlScanState state,
bool semicolon)
{
PQExpBufferData mybuf;
+#ifdef USE_ASSERT_CHECKING
int lexresult;
+#endif
char local_quote;
/* Must be scanning already */
@@ -1505,7 +1507,10 @@ psql_scan_slash_option(PsqlScanState state,
BEGIN(xslashargstart);
/* And lex. */
- lexresult = yylex();
+#ifdef USE_ASSERT_CHECKING
+ lexresult =
+#endif
+ yylex();
/*
* Check the lex result: we should have gotten back either LEXRES_OK
pg-cassert-unused-void.patchtext/x-patch; charset=UTF-8; name=pg-cassert-unused-void.patchDownload
diff --git i/src/backend/access/common/heaptuple.c w/src/backend/access/common/heaptuple.c
index 08d2b21..e5dbcc1 100644
--- i/src/backend/access/common/heaptuple.c
+++ w/src/backend/access/common/heaptuple.c
@@ -140,10 +140,7 @@ heap_fill_tuple(TupleDesc tupleDesc,
int i;
int numberOfAttributes = tupleDesc->natts;
Form_pg_attribute *att = tupleDesc->attrs;
-
-#ifdef USE_ASSERT_CHECKING
char *start = data;
-#endif
if (bit != NULL)
{
diff --git i/src/backend/executor/execGrouping.c w/src/backend/executor/execGrouping.c
index bd7973e..2151f19 100644
--- i/src/backend/executor/execGrouping.c
+++ w/src/backend/executor/execGrouping.c
@@ -540,10 +540,7 @@ static int
TupleHashTableMatch(const void *key1, const void *key2, Size keysize)
{
MinimalTuple tuple1 = ((const TupleHashEntryData *) key1)->firstTuple;
-
-#ifdef USE_ASSERT_CHECKING
MinimalTuple tuple2 = ((const TupleHashEntryData *) key2)->firstTuple;
-#endif
TupleTableSlot *slot1;
TupleTableSlot *slot2;
TupleHashTable hashtable = CurTupleHashTable;
diff --git i/src/backend/replication/syncrep.c w/src/backend/replication/syncrep.c
index 6bf69f0..3ec5a06 100644
--- i/src/backend/replication/syncrep.c
+++ w/src/backend/replication/syncrep.c
@@ -72,9 +72,7 @@ static void SyncRepCancelWait(void);
static int SyncRepGetStandbyPriority(void);
-#ifdef USE_ASSERT_CHECKING
static bool SyncRepQueueIsOrderedByLSN(void);
-#endif
/*
* ===========================================================
@@ -603,7 +601,6 @@ SyncRepUpdateSyncStandbysDefined(void)
}
}
-#ifdef USE_ASSERT_CHECKING
static bool
SyncRepQueueIsOrderedByLSN(void)
{
@@ -635,7 +632,6 @@ SyncRepQueueIsOrderedByLSN(void)
return true;
}
-#endif
/*
* ===========================================================
diff --git i/src/bin/psql/common.h w/src/bin/psql/common.h
index 8037cbc..1ae5671 100644
--- i/src/bin/psql/common.h
+++ w/src/bin/psql/common.h
@@ -16,7 +16,7 @@
#include <assert.h>
#define psql_assert(p) assert(p)
#else
-#define psql_assert(p)
+#define psql_assert(p) ((void)(p))
#endif
#define atooid(x) ((Oid) strtoul((x), NULL, 10))
diff --git i/src/include/postgres.h w/src/include/postgres.h
index c429f29..05b2d65 100644
--- i/src/include/postgres.h
+++ w/src/include/postgres.h
@@ -665,7 +665,7 @@ extern PGDLLIMPORT bool assert_enabled;
__FILE__, __LINE__))))
#ifndef USE_ASSERT_CHECKING
-#define Assert(condition)
+#define Assert(condition) ((void)(condition))
#define AssertMacro(condition) ((void)true)
#define AssertArg(condition)
#define AssertState(condition)
On Sat, Jan 21, 2012 at 1:06 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
So, here are three patches that could solve this issue.
pg-cassert-unused-attribute.patch, the one I already showed, using
__attribute__((unused).pg-cassert-unused-ifdef.patch, using only additional #ifdef
USE_ASSERT_CHECKING. This does have additional documentation value, but
you can see that it gets bulky and complicated. (I introduced several
bugs merely while creating this patch.)pg-cassert-unused-void.patch is an alternative approach that avoids the
warnings by casting the arguments of Assert() to void if assertions are
disabled. This has the least code impact, but it changes the
traditional semantics of asserts, which is that they disappear
completely when not enabled. You can see how this creates a problem in
src/backend/replication/syncrep.c, where the nontrivial call to
SyncRepQueueIsOrderedByLSN() now becomes visible even in non-assert
builds. I played around with some other options like
__attribute__((pure)) to make the compiler optimize the function away in
that case, but that didn't appear to work. So this might not be a
workable solution (and it would be GCC-specific anyway).
I think the third approach is unacceptable from a performance point of view.
Some of these problems can be fixed without resorting to as much
hackery as you have here. For example, in nodeWorkTableScan.c, you
could easily fix the problem by getting rid of the estate variable and
replacing its single use within the assertion with the expression from
used to initialize it on the previous line. I think this might the
cleanest solution in general.
I'm not sure what to do about the cases where that isn't practical.
Spraying the code with __attribute__((unused)) is somewhat undesirable
because it could mask a failure to properly initialize the variable in
an assert-enabled build. We could have a macro
PG_UNUSED_IF_NO_ASSERTS or something, but that doesn't have any
obvious advantage over just getting rid of the variable altogether in
such cases. I lean toward the view that variables not needed in
assertion-free builds should be #ifdef'd out altogether, as in your
second patch, but we should try to minimize the number of places where
we need to do that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
Spraying the code with __attribute__((unused)) is somewhat undesirable
because it could mask a failure to properly initialize the variable in
an assert-enabled build.
Ouch. Is it really true that __attribute__((unused)) disables detection
of use of uninitialized variables? That would be nasty, and it's not
obvious to me why it should need to work like that. But if it is true,
then I agree that that makes this approach not too tenable.
regards, tom lane
On Tue, Jan 24, 2012 at 12:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Spraying the code with __attribute__((unused)) is somewhat undesirable
because it could mask a failure to properly initialize the variable in
an assert-enabled build.Ouch. Is it really true that __attribute__((unused)) disables detection
of use of uninitialized variables? That would be nasty, and it's not
obvious to me why it should need to work like that. But if it is true,
then I agree that that makes this approach not too tenable.
Oh, I think maybe I am confused. The downsides of disabling *unused*
variable detection are obviously much less than the downsides of
disabling *uninitialized* variable declaration... although neither is
ideal.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jan 24, 2012 at 12:12 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ouch. Is it really true that __attribute__((unused)) disables detection
of use of uninitialized variables?
Oh, I think maybe I am confused. The downsides of disabling *unused*
variable detection are obviously much less than the downsides of
disabling *uninitialized* variable declaration... although neither is
ideal.
OK. MHO is that __attribute__((unused)) is probably less annoying than
#ifdef overall. Also, it occurs to me that an intermediate macro
"PG_USED_FOR_ASSERTS_ONLY" would be a good idea, first because it
documents *why* you want to mark the variable as possibly unused,
and second because changing the macro definition would provide an easy way
to check for totally-unused variables, in case we wanted to periodically
make such checks.
This is all modulo the question of what pgindent will do with it,
which I would still like to see tested before we commit to a method.
regards, tom lane
I wrote:
Also, it occurs to me that an intermediate macro
"PG_USED_FOR_ASSERTS_ONLY" would be a good idea, first because it
documents *why* you want to mark the variable as possibly unused,
and second because changing the macro definition would provide an easy way
to check for totally-unused variables, in case we wanted to periodically
make such checks.
Uh, wait a second. Why not
#ifdef USE_ASSERT_CHECKING
#define PG_USED_FOR_ASSERTS_ONLY
#else
#define PG_USED_FOR_ASSERTS_ONLY __attribute__((unused))
#endif
Then, when you build with asserts on, you *automatically* get told
if the variable is entirely unused.
regards, tom lane
On Tue, Jan 24, 2012 at 1:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
Also, it occurs to me that an intermediate macro
"PG_USED_FOR_ASSERTS_ONLY" would be a good idea, first because it
documents *why* you want to mark the variable as possibly unused,
and second because changing the macro definition would provide an easy way
to check for totally-unused variables, in case we wanted to periodically
make such checks.Uh, wait a second. Why not
#ifdef USE_ASSERT_CHECKING
#define PG_USED_FOR_ASSERTS_ONLY
#else
#define PG_USED_FOR_ASSERTS_ONLY __attribute__((unused))
#endifThen, when you build with asserts on, you *automatically* get told
if the variable is entirely unused.
Yes, that's what I meant when I suggested it originally. I'm just not
sure it's any nicer than adding ifdefs for USE_ASSERT_CHECKING.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
Yes, that's what I meant when I suggested it originally. I'm just not
sure it's any nicer than adding ifdefs for USE_ASSERT_CHECKING.
I'm inclined to think that it probably is nicer, just because of less
vertical space used. But again, this opinion is contingent on what it
will look like after pgindent gets done with it ...
regards, tom lane
On tis, 2012-01-24 at 13:18 -0500, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Yes, that's what I meant when I suggested it originally. I'm just not
sure it's any nicer than adding ifdefs for USE_ASSERT_CHECKING.I'm inclined to think that it probably is nicer, just because of less
vertical space used. But again, this opinion is contingent on what it
will look like after pgindent gets done with it ...
Here is a demo diff of what pgindent would do with the various
approaches (btw., nice job on making pgindent easy to use for everyone
now).
As you can see, pgindent adds whitespace on top of #ifdef
USE_ASSERT_CHECKING, and messes up the vertical alignment of variable
definitions that contain extra attributes.
All things considered, I like the PG_USED_FOR_ASSERTS_ONLY solution best.
Attachments:
pg-cassert-unused-pgindent-demo.difftext/x-patch; charset=UTF-8; name=pg-cassert-unused-pgindent-demo.diffDownload
diff --git i/src/backend/access/hash/hashovfl.c w/src/backend/access/hash/hashovfl.c
index 130c296..b61c8ee 100644
--- i/src/backend/access/hash/hashovfl.c
+++ w/src/backend/access/hash/hashovfl.c
@@ -391,7 +391,10 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
uint32 ovflbitno;
int32 bitmappage,
bitmapbit;
+
+#ifdef USE_ASSERT_CHECKING
Bucket bucket;
+#endif
/* Get information from the doomed page */
_hash_checkpage(rel, ovflbuf, LH_OVERFLOW_PAGE);
@@ -400,7 +403,9 @@ _hash_freeovflpage(Relation rel, Buffer ovflbuf,
ovflopaque = (HashPageOpaque) PageGetSpecialPointer(ovflpage);
nextblkno = ovflopaque->hasho_nextblkno;
prevblkno = ovflopaque->hasho_prevblkno;
+#ifdef USE_ASSERT_CHECKING
bucket = ovflopaque->hasho_bucket;
+#endif
/*
* Zero the page for debugging's sake; then write and release it. (Note:
diff --git i/src/backend/executor/execCurrent.c w/src/backend/executor/execCurrent.c
index b07161f..2c8929b 100644
--- i/src/backend/executor/execCurrent.c
+++ w/src/backend/executor/execCurrent.c
@@ -151,7 +151,7 @@ execCurrentOf(CurrentOfExpr *cexpr,
{
ScanState *scanstate;
bool lisnull;
- Oid tuple_tableoid;
+ Oid tuple_tableoid PG_USED_FOR_ASSERTS_ONLY;
ItemPointer tuple_tid;
/*
diff --git i/src/backend/executor/nodeMaterial.c w/src/backend/executor/nodeMaterial.c
index b320b54..3a6bfec 100644
--- i/src/backend/executor/nodeMaterial.c
+++ w/src/backend/executor/nodeMaterial.c
@@ -66,7 +66,7 @@ ExecMaterial(MaterialState *node)
* Allocate a second read pointer to serve as the mark. We know it
* must have index 1, so needn't store that.
*/
- int ptrno;
+ int ptrno PG_USED_FOR_ASSERTS_ONLY;
ptrno = tuplestore_alloc_read_pointer(tuplestorestate,
node->eflags);
diff --git i/src/backend/executor/nodeSetOp.c w/src/backend/executor/nodeSetOp.c
index 7fa5730..ad2e80d 100644
--- i/src/backend/executor/nodeSetOp.c
+++ w/src/backend/executor/nodeSetOp.c
@@ -344,7 +344,7 @@ setop_fill_hash_table(SetOpState *setopstate)
SetOp *node = (SetOp *) setopstate->ps.plan;
PlanState *outerPlan;
int firstFlag;
- bool in_first_rel;
+ bool in_first_rel __attribute__((unused));
/*
* get state info from node
diff --git i/src/include/c.h w/src/include/c.h
index 82acd14..2dd5c67 100644
--- i/src/include/c.h
+++ w/src/include/c.h
@@ -850,4 +850,10 @@ extern int fdatasync(int fildes);
/* /port compatibility functions */
#include "port.h"
+#ifdef USE_ASSERT_CHECKING
+#define PG_USED_FOR_ASSERTS_ONLY
+#else
+#define PG_USED_FOR_ASSERTS_ONLY __attribute__((unused))
+#endif
+
#endif /* C_H */
Peter Eisentraut <peter_e@gmx.net> writes:
As you can see, pgindent adds whitespace on top of #ifdef
USE_ASSERT_CHECKING, and messes up the vertical alignment of variable
definitions that contain extra attributes.
Hm. I bet it thinks that PG_USED_FOR_ASSERTS_ONLY is the variable name,
which means that the behavior might be more exciting for multi-word type
names (for instance "struct foo" or "volatile int *". Could you check
a few cases like that?
All things considered, I like the PG_USED_FOR_ASSERTS_ONLY solution best.
I agree, unless the more complicated cases go further off the rails.
regards, tom lane
On tis, 2012-03-20 at 15:04 -0400, Tom Lane wrote:
Hm. I bet it thinks that PG_USED_FOR_ASSERTS_ONLY is the variable
name, which means that the behavior might be more exciting for
multi-word type names (for instance "struct foo" or "volatile int *".
Could you check a few cases like that?
Tested, doesn't make a difference. Hence committed that way.