From a9284849b48b04fa2836aaf704659974c13e610d Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 3 Apr 2016 14:17:20 -0400
Subject: [PATCH] Clean up some stuff in new contrib/bloom module.
Coverity complained about implicit sign-extension in the
BloomPageGetFreeSpace macro, probably because sizeOfBloomTuple isn't wide
enough for size calculations. No overflow is really possible as long as
maxoff and sizeOfBloomTuple are small enough to represent a realistic
situation, but it seems like a good idea to declare sizeOfBloomTuple as
Size not int32.
Add missing check on BloomPageAddItem() result, again from Coverity.
Avoid core dump due to not allocating so->sign array when
scan->numberOfKeys is zero. Also thanks to Coverity.
Use FLEXIBLE_ARRAY_MEMBER rather than declaring an array as size 1
when it isn't necessarily.
Very minor beautification of related code.
Unfortunately, none of the Coverity-detected mistakes look like they
could account for the remaining buildfarm unhappiness with this
module. It's barely possible that the FLEXIBLE_ARRAY_MEMBER mistake
does account for that, if it's enabling bogus compiler optimizations;
but I'm not terribly optimistic. We probably still have bugs to
find here.
---
contrib/bloom/blinsert.c | 11 ++++++++---
contrib/bloom/bloom.h | 4 ++--
contrib/bloom/blscan.c | 7 +++++--
3 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index 9e6678087ca..6eecb12187d 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -96,10 +96,10 @@ bloomBuildCallback(Relation index, HeapTuple htup, Datum *values,
initCachedPage(buildstate);
- if (BloomPageAddItem(&buildstate->blstate, buildstate->data, itup) == false)
+ if (!BloomPageAddItem(&buildstate->blstate, buildstate->data, itup))
{
/* We shouldn't be here since we're inserting to the empty page */
- elog(ERROR, "can not add new tuple");
+ elog(ERROR, "could not add new bloom tuple to empty page");
}
}
@@ -298,7 +298,12 @@ blinsert(Relation index, Datum *values, bool *isnull,
metaData = BloomPageGetMeta(metaPage);
page = GenericXLogRegister(state, buffer, true);
BloomInitPage(page, 0);
- BloomPageAddItem(&blstate, page, itup);
+
+ if (!BloomPageAddItem(&blstate, page, itup))
+ {
+ /* We shouldn't be here since we're inserting to the empty page */
+ elog(ERROR, "could not add new bloom tuple to empty page");
+ }
metaData->nStart = 0;
metaData->nEnd = 1;
diff --git a/contrib/bloom/bloom.h b/contrib/bloom/bloom.h
index fb0bc07f284..8f3881d8446 100644
--- a/contrib/bloom/bloom.h
+++ b/contrib/bloom/bloom.h
@@ -118,7 +118,7 @@ typedef struct BloomState
* sizeOfBloomTuple is index's specific, and it depends on reloptions, so
* precompute it
*/
- int32 sizeOfBloomTuple;
+ Size sizeOfBloomTuple;
} BloomState;
#define BloomPageGetFreeSpace(state, page) \
@@ -134,7 +134,7 @@ typedef uint16 SignType;
typedef struct BloomTuple
{
ItemPointerData heapPtr;
- SignType sign[1];
+ SignType sign[FLEXIBLE_ARRAY_MEMBER];
} BloomTuple;
#define BLOOMTUPLEHDRSZ offsetof(BloomTuple, sign)
diff --git a/contrib/bloom/blscan.c b/contrib/bloom/blscan.c
index d156e88669a..6e3cb84bb11 100644
--- a/contrib/bloom/blscan.c
+++ b/contrib/bloom/blscan.c
@@ -94,7 +94,7 @@ blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
BufferAccessStrategy bas;
BloomScanOpaque so = (BloomScanOpaque) scan->opaque;
- if (so->sign == NULL && scan->numberOfKeys > 0)
+ if (so->sign == NULL)
{
/* New search: have to calculate search signature */
ScanKey skey = scan->keyData;
@@ -151,10 +151,13 @@ blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm)
bool res = true;
/* Check index signature with scan signature */
- for (i = 0; res && i < so->state.opts->bloomLength; i++)
+ for (i = 0; i < so->state.opts->bloomLength; i++)
{
if ((itup->sign[i] & so->sign[i]) != so->sign[i])
+ {
res = false;
+ break;
+ }
}
/* Add matching tuples to bitmap */
--
GitLab