On Thu, 28 Dec 2023 at 23:21, David Rowley <[email protected]> wrote:
> then instead of having to do:
>
> #ifdef REALLOCATE_BITMAPSETS
> a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
> memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
> pfree(tmp);
> #endif
>
> all over the place. Couldn't we do:
>
> #ifdef REALLOCATE_BITMAPSETS
> return bms_clone_and_free(a);
> #else
> return a;
> #endif
I looked into this a bit more and I just can't see why the current
code feels like it must do the reallocation in functions such as
bms_del_members(). We don't repalloc the set there, ever, so why do
we need to do it when building with REALLOCATE_BITMAPSETS? It seems
to me the point of REALLOCATE_BITMAPSETS is to ensure that *if* we
possibly could end up reallocating the set that we *do* reallocate.
There's also a few cases where you could argue that the
REALLOCATE_BITMAPSETS code has introduced bugs. For example,
bms_del_members(), bms_join() and bms_int_members() are meant to
guarantee that their left input (both inputs in the case of
bms_join()) are recycled. Compiling with REALLOCATE_BITMAPSETS
invalidates that, so it seems about as likely that building with
REALLOCATE_BITMAPSETS could cause bugs rather than find bugs.
I've hacked a bit on this to fix these problems and also added some
documentation to try to offer future people modifying bitmapset.c some
guidance on if they need to care about REALLOCATE_BITMAPSETS.
I also don't think "hangling" is a word. So I've adjusted the comment
in pg_config_manual.h to fix that.
David
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index e13ecaa155..e0eeefa8bd 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -16,6 +16,15 @@
* containing only a single word (likely the majority of them) this halves the
* number of loop condition checks.
*
+ * Because manipulation of a bitmapset can often be done without having to
+ * reallocate the memory for the set, we support REALLOCATE_BITMAPSETS as a
+ * debug option to have the code *always* reallocate the bitmapset when the
+ * set *could* be reallocated as a result of the modification. This increases
+ * the likelihood of finding bugs in cases where there's more than one
+ * reference to a set and the additional ones are left pointing to freed
+ * memory. Such bugs are unlikely to be otherwise found by our regression
+ * tests as the vast majority of bitmapsets will only ever contain a single
+ * bitmapword.
*
* Copyright (c) 2003-2023, PostgreSQL Global Development Group
*
@@ -72,6 +81,45 @@
#error "invalid BITS_PER_BITMAPWORD"
#endif
+#ifdef USE_ASSERT_CHECKING
+/*
+ * bms_is_valid_set - for cassert builds to check for valid sets
+ */
+static bool
+bms_is_valid_set(const Bitmapset *a)
+{
+ /* NULL is the correct representation of an empty set */
+ if (a == NULL)
+ return true;
+
+ /* check the node tag is set correctly. Freed pointer, maybe? */
+ if (!IsA(a, Bitmapset))
+ return false;
+
+ /* trailing zero words are not allowed */
+ if (a->words[a->nwords - 1] == 0)
+ return false;
+
+ return true;
+}
+#endif
+
+#ifdef REALLOCATE_BITMAPSETS
+/*
+ * bms_copy_and_free
+ * Only required in REALLOCATE_BITMAPSETS builds. Provide a
simple way to
+ * return a freshly allocated set similar to what would happen if
the set
+ * had to be enlarged to add additional words.
+ */
+static Bitmapset *
+bms_copy_and_free(Bitmapset *a)
+{
+ Bitmapset *c = bms_copy(a);
+
+ bms_free(a);
+ return c;
+}
+#endif
/*
* bms_copy - make a palloc'd copy of a bitmapset
@@ -82,9 +130,11 @@ bms_copy(const Bitmapset *a)
Bitmapset *result;
size_t size;
+ Assert(bms_is_valid_set(a));
+
if (a == NULL)
return NULL;
- Assert(IsA(a, Bitmapset));
+
size = BITMAPSET_SIZE(a->nwords);
result = (Bitmapset *) palloc(size);
memcpy(result, a, size);
@@ -99,8 +149,8 @@ bms_equal(const Bitmapset *a, const Bitmapset *b)
{
int i;
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -140,8 +190,8 @@ bms_compare(const Bitmapset *a, const Bitmapset *b)
{
int i;
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -216,8 +266,8 @@ bms_union(const Bitmapset *a, const Bitmapset *b)
int otherlen;
int i;
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -257,8 +307,8 @@ bms_intersect(const Bitmapset *a, const Bitmapset *b)
int resultlen;
int i;
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL || b == NULL)
@@ -307,8 +357,8 @@ bms_difference(const Bitmapset *a, const Bitmapset *b)
Bitmapset *result;
int i;
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -316,8 +366,6 @@ bms_difference(const Bitmapset *a, const Bitmapset *b)
if (b == NULL)
return bms_copy(a);
- Assert(IsA(a, Bitmapset) && IsA(b, Bitmapset));
-
/*
* In Postgres' usage, an empty result is a very common case, so it's
* worth optimizing for that by testing bms_nonempty_difference(). This
@@ -374,8 +422,8 @@ bms_is_subset(const Bitmapset *a, const Bitmapset *b)
{
int i;
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -383,8 +431,6 @@ bms_is_subset(const Bitmapset *a, const Bitmapset *b)
if (b == NULL)
return false;
- Assert(IsA(a, Bitmapset) && IsA(b, Bitmapset));
-
/* 'a' can't be a subset of 'b' if it contains more words */
if (a->nwords > b->nwords)
return false;
@@ -411,8 +457,8 @@ bms_subset_compare(const Bitmapset *a, const Bitmapset *b)
int shortlen;
int i;
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -424,8 +470,6 @@ bms_subset_compare(const Bitmapset *a, const Bitmapset *b)
if (b == NULL)
return BMS_SUBSET2;
- Assert(IsA(a, Bitmapset) && IsA(b, Bitmapset));
-
/* Check common words */
result = BMS_EQUAL; /* status so far */
shortlen = Min(a->nwords, b->nwords);
@@ -477,14 +521,14 @@ bms_is_member(int x, const Bitmapset *a)
int wordnum,
bitnum;
+ Assert(bms_is_valid_set(a));
+
/* XXX better to just return false for x<0 ? */
if (x < 0)
elog(ERROR, "negative bitmapset member not allowed");
if (a == NULL)
return false;
- Assert(IsA(a, Bitmapset));
-
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
if (wordnum >= a->nwords)
@@ -509,12 +553,12 @@ bms_member_index(Bitmapset *a, int x)
int result = 0;
bitmapword mask;
+ Assert(bms_is_valid_set(a));
+
/* return -1 if not a member of the bitmap */
if (!bms_is_member(x, a))
return -1;
- Assert(IsA(a, Bitmapset));
-
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
@@ -549,8 +593,8 @@ bms_overlap(const Bitmapset *a, const Bitmapset *b)
int shortlen;
int i;
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL || b == NULL)
@@ -576,7 +620,7 @@ bms_overlap_list(const Bitmapset *a, const List *b)
int wordnum,
bitnum;
- Assert(a == NULL || IsA(a, Bitmapset));
+ Assert(bms_is_valid_set(a));
if (a == NULL || b == NIL)
return false;
@@ -607,8 +651,8 @@ bms_nonempty_difference(const Bitmapset *a, const Bitmapset
*b)
{
int i;
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -640,11 +684,11 @@ bms_singleton_member(const Bitmapset *a)
int nwords;
int wordnum;
+ Assert(bms_is_valid_set(a));
+
if (a == NULL)
elog(ERROR, "bitmapset is empty");
- Assert(IsA(a, Bitmapset));
-
nwords = a->nwords;
wordnum = 0;
do
@@ -683,9 +727,11 @@ bms_get_singleton_member(const Bitmapset *a, int *member)
int nwords;
int wordnum;
+ Assert(bms_is_valid_set(a));
+
if (a == NULL)
return false;
- Assert(IsA(a, Bitmapset));
+
nwords = a->nwords;
wordnum = 0;
do
@@ -717,9 +763,11 @@ bms_num_members(const Bitmapset *a)
int nwords;
int wordnum;
+ Assert(bms_is_valid_set(a));
+
if (a == NULL)
return 0;
- Assert(IsA(a, Bitmapset));
+
nwords = a->nwords;
wordnum = 0;
do
@@ -745,9 +793,11 @@ bms_membership(const Bitmapset *a)
int nwords;
int wordnum;
+ Assert(bms_is_valid_set(a));
+
if (a == NULL)
return BMS_EMPTY_SET;
- Assert(IsA(a, Bitmapset));
+
nwords = a->nwords;
wordnum = 0;
do
@@ -786,11 +836,13 @@ bms_add_member(Bitmapset *a, int x)
int wordnum,
bitnum;
+ Assert(bms_is_valid_set(a));
+
if (x < 0)
elog(ERROR, "negative bitmapset member not allowed");
if (a == NULL)
return bms_make_singleton(x);
- Assert(IsA(a, Bitmapset));
+
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
@@ -799,15 +851,8 @@ bms_add_member(Bitmapset *a, int x)
{
int oldnwords = a->nwords;
int i;
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(wordnum + 1));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#else
a = (Bitmapset *) repalloc(a, BITMAPSET_SIZE(wordnum + 1));
-#endif
a->nwords = wordnum + 1;
/* zero out the enlarged portion */
i = oldnwords;
@@ -816,19 +861,14 @@ bms_add_member(Bitmapset *a, int x)
a->words[i] = 0;
} while (++i < a->nwords);
}
-#ifdef REALLOCATE_BITMAPSETS
- else
- {
- Bitmapset *tmp = a;
-
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
- }
-#endif
a->words[wordnum] |= ((bitmapword) 1 << bitnum);
+
+#ifdef REALLOCATE_BITMAPSETS
+ return bms_copy_and_free(a);
+#else
return a;
+#endif
}
/*
@@ -843,26 +883,17 @@ bms_del_member(Bitmapset *a, int x)
{
int wordnum,
bitnum;
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
-#endif
+
+ Assert(bms_is_valid_set(a));
if (x < 0)
elog(ERROR, "negative bitmapset member not allowed");
if (a == NULL)
return NULL;
- Assert(IsA(a, Bitmapset));
-
wordnum = WORDNUM(x);
bitnum = BITNUM(x);
-#ifdef REALLOCATE_BITMAPSETS
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#endif
-
/* member can't exist. Return 'a' unmodified */
if (unlikely(wordnum >= a->nwords))
return a;
@@ -900,8 +931,8 @@ bms_add_members(Bitmapset *a, const Bitmapset *b)
int otherlen;
int i;
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -916,13 +947,6 @@ bms_add_members(Bitmapset *a, const Bitmapset *b)
}
else
{
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
-
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#endif
result = a;
other = b;
}
@@ -935,7 +959,12 @@ bms_add_members(Bitmapset *a, const Bitmapset *b)
} while (++i < otherlen);
if (result != a)
pfree(a);
+
+#ifdef REALLOCATE_BITMAPSETS
+ return bms_copy_and_free(result);
+#else
return result;
+#endif
}
/*
@@ -955,7 +984,7 @@ bms_add_range(Bitmapset *a, int lower, int upper)
ushiftbits,
wordnum;
- Assert(a == NULL || IsA(a, Bitmapset));
+ Assert(bms_is_valid_set(a));
/* do nothing if nothing is called for, without further checking */
if (upper < lower)
@@ -975,16 +1004,9 @@ bms_add_range(Bitmapset *a, int lower, int upper)
{
int oldnwords = a->nwords;
int i;
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(uwordnum + 1));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#else
/* ensure we have enough words to store the upper bit */
a = (Bitmapset *) repalloc(a, BITMAPSET_SIZE(uwordnum + 1));
-#endif
a->nwords = uwordnum + 1;
/* zero out the enlarged portion */
i = oldnwords;
@@ -1021,7 +1043,11 @@ bms_add_range(Bitmapset *a, int lower, int upper)
a->words[uwordnum] |= (~(bitmapword) 0) >> ushiftbits;
}
+#ifdef REALLOCATE_BITMAPSETS
+ return bms_copy_and_free(a);
+#else
return a;
+#endif
}
/*
@@ -1033,15 +1059,9 @@ bms_int_members(Bitmapset *a, const Bitmapset *b)
int lastnonzero;
int shortlen;
int i;
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
-#endif
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
-
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -1052,12 +1072,6 @@ bms_int_members(Bitmapset *a, const Bitmapset *b)
return NULL;
}
-#ifdef REALLOCATE_BITMAPSETS
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#endif
-
/* Intersect b into a; we need never copy */
shortlen = Min(a->nwords, b->nwords);
lastnonzero = -1;
@@ -1089,12 +1103,9 @@ Bitmapset *
bms_del_members(Bitmapset *a, const Bitmapset *b)
{
int i;
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
-#endif
- Assert(a == NULL || (IsA(a, Bitmapset) && a->words[a->nwords - 1] !=
0));
- Assert(b == NULL || (IsA(b, Bitmapset) && b->words[b->nwords - 1] !=
0));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -1102,12 +1113,6 @@ bms_del_members(Bitmapset *a, const Bitmapset *b)
if (b == NULL)
return a;
-#ifdef REALLOCATE_BITMAPSETS
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#endif
-
/* Remove b's bits from a; we need never copy */
if (a->nwords > b->nwords)
{
@@ -1160,15 +1165,9 @@ bms_join(Bitmapset *a, Bitmapset *b)
Bitmapset *other;
int otherlen;
int i;
-#ifdef REALLOCATE_BITMAPSETS
- Bitmapset *tmp = a;
-#endif
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
-
- Assert(a == NULL || IsA(a, Bitmapset));
- Assert(b == NULL || IsA(b, Bitmapset));
+ Assert(bms_is_valid_set(a));
+ Assert(bms_is_valid_set(b));
/* Handle cases where either input is NULL */
if (a == NULL)
@@ -1176,12 +1175,6 @@ bms_join(Bitmapset *a, Bitmapset *b)
if (b == NULL)
return a;
-#ifdef REALLOCATE_BITMAPSETS
- a = (Bitmapset *) palloc(BITMAPSET_SIZE(tmp->nwords));
- memcpy(a, tmp, BITMAPSET_SIZE(tmp->nwords));
- pfree(tmp);
-#endif
-
/* Identify shorter and longer input; use longer one as result */
if (a->nwords < b->nwords)
{
@@ -1231,7 +1224,7 @@ bms_next_member(const Bitmapset *a, int prevbit)
int wordnum;
bitmapword mask;
- Assert(a == NULL || IsA(a, Bitmapset));
+ Assert(bms_is_valid_set(a));
if (a == NULL)
return -2;
@@ -1292,7 +1285,7 @@ bms_prev_member(const Bitmapset *a, int prevbit)
int ushiftbits;
bitmapword mask;
- Assert(a == NULL || IsA(a, Bitmapset));
+ Assert(bms_is_valid_set(a));
/*
* If set is NULL or if there are no more bits to the right then we've
@@ -1337,6 +1330,8 @@ bms_prev_member(const Bitmapset *a, int prevbit)
uint32
bms_hash_value(const Bitmapset *a)
{
+ Assert(bms_is_valid_set(a));
+
if (a == NULL)
return 0; /* All empty sets hash
to 0 */
return DatumGetUInt32(hash_any((const unsigned char *) a->words,
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index 16c383ba7f..3920a535d2 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -336,8 +336,9 @@
/* #define COPY_PARSE_PLAN_TREES */
/*
- * Define this to force Bitmapset reallocation on each modification. Helps
- * to find hangling pointers to Bitmapset's.
+ * Define this to force Bitmapset reallocation on modifications which *could*
+ * result in the set having to be reallocated. This helps find dangling
+ * pointers to Bitmapset's.
*/
/* #define REALLOCATE_BITMAPSETS */
