On 12/25/17 00:32, Michael Paquier wrote:
>> So here is a minimal patch set to perhaps wrap this up for the time
>> being. I have added static assertions that check the sizes of
>> GinNullCategory and GinTernaryValue, which I think are the two critical
>> places that require compatibility with bool. And then we include
>> <stdbool.h> only if its bool has size 1.
>
> + /*
> + * now we can use the nullFlags as category codes
> + */
> + StaticAssertStmt(sizeof(GinNullCategory) == sizeof(bool),
> + "sizes of GinNullCategory and bool are not equal");
> *categories = (GinNullCategory *) nullFlags;
>
> Hm. So on powerpc compilation is going to fail with this patch as
> sizeof(char) is 1, no?
Yes, but right now it would (probably) just fail in mysterious ways, so
the static assertion adds safety.
> Wouldn't it be better to just allocate an array
> for GinNullCategory entries and then just fill in the values one by
> one with GIN_CAT_NORM_KEY or GIN_CAT_NULL_KEY by scanning nullFlags?
Yeah, initially I though making another loop through the array would be
adding more overhead. But reading the code again, we already loop
through the array anyway to set the nullFlags to the right bit patterns.
So I think we can just drop that and build a proper GinNullCategory
array instead. I think that would be much cleaner. Patch attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From dc47c84286345eae48582df42bd977e369b4f341 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 26 Dec 2017 13:47:18 -0500
Subject: [PATCH] Don't cast between GinNullCategory and bool
The original idea was that we could use a isNull-style bool array
directly as a GinNullCategory array. However, the existing code already
acknowledges that that doesn't really work, because of the possibility
that bool as currently defined can have arbitrary bit patterns for true
values. So it has to loop through the nullFlags array to set each bool
value to and acceptable value. But if we are looping through the whole
array anyway, we might as well build a proper GinNullCategory array
instead and abandon the type casting. That makes the code much safer in
case bool is ever changed to something else.
---
src/backend/access/gin/ginscan.c | 19 ++++++++-----------
src/backend/access/gin/ginutil.c | 18 ++++++++----------
src/include/access/ginblock.h | 7 +++++--
3 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/src/backend/access/gin/ginscan.c b/src/backend/access/gin/ginscan.c
index 7ceea7a741..77c0c577b5 100644
--- a/src/backend/access/gin/ginscan.c
+++ b/src/backend/access/gin/ginscan.c
@@ -295,6 +295,7 @@ ginNewScanKey(IndexScanDesc scan)
bool *partial_matches = NULL;
Pointer *extra_data = NULL;
bool *nullFlags = NULL;
+ GinNullCategory *categories;
int32 searchMode = GIN_SEARCH_MODE_DEFAULT;
/*
@@ -346,15 +347,12 @@ ginNewScanKey(IndexScanDesc scan)
}
/*
- * If the extractQueryFn didn't create a nullFlags array,
create one,
- * assuming that everything's non-null. Otherwise, run through
the
- * array and make sure each value is exactly 0 or 1; this
ensures
- * binary compatibility with the GinNullCategory
representation. While
- * at it, detect whether any null keys are present.
+ * Create GinNullCategory representation. If the extractQueryFn
+ * didn't create a nullFlags array, we assume everything is
non-null.
+ * While at it, detect whether any null keys are present.
*/
- if (nullFlags == NULL)
- nullFlags = (bool *) palloc0(nQueryValues *
sizeof(bool));
- else
+ categories = (GinNullCategory *) palloc0(nQueryValues *
sizeof(GinNullCategory));
+ if (nullFlags)
{
int32 j;
@@ -362,17 +360,16 @@ ginNewScanKey(IndexScanDesc scan)
{
if (nullFlags[j])
{
- nullFlags[j] = true; /* not any
other nonzero value */
+ categories[j] = GIN_CAT_NULL_KEY;
hasNullQuery = true;
}
}
}
- /* now we can use the nullFlags as category codes */
ginFillScanKey(so, skey->sk_attno,
skey->sk_strategy, searchMode,
skey->sk_argument, nQueryValues,
- queryValues, (GinNullCategory *)
nullFlags,
+ queryValues, categories,
partial_matches, extra_data);
}
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index d9c6483437..41d4b4fb6f 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -529,19 +529,10 @@ ginExtractEntries(GinState *ginstate, OffsetNumber attnum,
/*
* If the extractValueFn didn't create a nullFlags array, create one,
- * assuming that everything's non-null. Otherwise, run through the
array
- * and make sure each value is exactly 0 or 1; this ensures binary
- * compatibility with the GinNullCategory representation.
+ * assuming that everything's non-null.
*/
if (nullFlags == NULL)
nullFlags = (bool *) palloc0(*nentries * sizeof(bool));
- else
- {
- for (i = 0; i < *nentries; i++)
- nullFlags[i] = (nullFlags[i] ? true : false);
- }
- /* now we can use the nullFlags as category codes */
- *categories = (GinNullCategory *) nullFlags;
/*
* If there's more than one key, sort and unique-ify.
@@ -600,6 +591,13 @@ ginExtractEntries(GinState *ginstate, OffsetNumber attnum,
pfree(keydata);
}
+ /*
+ * Create GinNullCategory representation from nullFlags.
+ */
+ *categories = (GinNullCategory *) palloc0(*nentries *
sizeof(GinNullCategory));
+ for (i = 0; i < *nentries; i++)
+ (*categories)[i] = (nullFlags[i] ? GIN_CAT_NULL_KEY :
GIN_CAT_NORM_KEY);
+
return entries;
}
diff --git a/src/include/access/ginblock.h b/src/include/access/ginblock.h
index 114370c7d7..c3af3f0380 100644
--- a/src/include/access/ginblock.h
+++ b/src/include/access/ginblock.h
@@ -188,8 +188,11 @@ typedef struct
/*
* Category codes to distinguish placeholder nulls from ordinary NULL keys.
- * Note that the datatype size and the first two code values are chosen to be
- * compatible with the usual usage of bool isNull flags.
+ *
+ * The first two code values were chosen to be compatible with the usual usage
+ * of bool isNull flags. However, casting between bool and GinNullCategory is
+ * risky because of the possibility of different bit patterns and type sizes,
+ * so it is no longer done.
*
* GIN_CAT_EMPTY_QUERY is never stored in the index; and notice that it is
* chosen to sort before not after regular key values.
--
2.15.1
