public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Greg Burd <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Wed, 1 Oct 2025 12:00:39 +1300
Message-ID: <CAApHDvrvixMmscGEfEyYz0=qbwzM7Y0Up-Uh_AKVmfn8K2sn_A@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAApHDvq-4994jpDnspQ+bgSbzo0+=Z4YQbj+YfP7wi97B0e1qg@mail.gmail.com>
<[email protected]>
On Wed, 1 Oct 2025 at 01:38, Greg Burd <[email protected]> wrote:
> v2 attached.
Thanks.
I looked at this and I think we can do a bit better to simplify the
code and standardise what we do when faced with bogus inputs.
Here's what I've done:
1. Added helper macros PG_ARG_GETBITMAPSET() and
PG_RETURN_BITMAPSET_AS_TEXT(). These themselves save a few hundred
lines of code and make the functions easier to read.
2. Standardised that the test function returns NULL when it receives
invalid input. e.g if we aim to pass an "int" to the underlying
Bitmapset function, if someone calls the test function with an SQL
NULL, just return NULL rather than trying to come up with some logic
on what we should do about that.
3. I couldn't quite understand the extra parameter to
test_bms_get_singleton_member() so I ended up removing it and making
the test function return -1 when the set isn't a singleton. I didn't
search for discussion about that so I might have missed the specific
reason it was done that way.
4. Wrote some comments to try to communicate the standards defined.
This aims to offer guidance to callers of these functions and anyone
hacking on the module itself in the future.
git diff --stat looks like:
4 files changed, 233 insertions(+), 505 deletions(-)
Things not done:
1. I didn't update the expected test results.
2. I didn't rationalise the tests. I feel that there's no point in the
tests that purposefully pass invalid input to the module's test
function. It appears that's aiming to increase coverage on the test
module's C code, which I don't quite understand. In any case, due to
the helper macros, I don't think removing these will lower the
per-source-file line coverage of the module itself.
I've not checked my work very thoroughly. I can do that, or you can if
you and Michael are ok with the proposed ideas. I'm also not aiming to
commandeer anything here. Writing the patch was just the easiest way
to communicate the ideas I had.
I also wonder how worthwhile it is to try to free the Bitmapsets. I'd
have expected these allocations to be in a pretty short-lived memory
context. I count 44 calls to bms_free().
David
Attachments:
[application/octet-stream] adjustments_to_test_bitmapset_module.patch (30.7K, 2-adjustments_to_test_bitmapset_module.patch)
download | inline diff:
diff --git a/src/test/modules/test_bitmapset/expected/test_bitmapset.out b/src/test/modules/test_bitmapset/expected/test_bitmapset.out
index be7b6399c82..2e204d687df 100644
--- a/src/test/modules/test_bitmapset/expected/test_bitmapset.out
+++ b/src/test/modules/test_bitmapset/expected/test_bitmapset.out
@@ -63,7 +63,7 @@ SELECT test_bms_add_member('(b 10)', 10) AS result;
SELECT test_bms_add_member('(b)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_replace_members()
@@ -76,7 +76,7 @@ SELECT test_bms_replace_members(NULL, '(b 1 2 3)') AS result;
SELECT test_bms_replace_members('(b 1 2 3)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_replace_members('(b 1 2 3)', '(b 3 5 6)') AS result;
@@ -108,13 +108,13 @@ SELECT test_bms_replace_members('(b 1 2 3 4 5)', '(b 500 600)') AS result;
SELECT test_bms_replace_members('(b 1 2 3)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_replace_members('(b 5)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_replace_members(NULL, '(b 5)') AS result;
@@ -126,7 +126,7 @@ SELECT test_bms_replace_members(NULL, '(b 5)') AS result;
SELECT test_bms_replace_members(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_del_member()
@@ -135,13 +135,13 @@ ERROR: negative bitmapset member not allowed
SELECT test_bms_del_member('(b)', 10) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_member('(b 10)', 10) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_member('(b 10)', 5) AS result;
@@ -193,26 +193,26 @@ SELECT test_bms_del_member('(b 1 50 100 200)', 100) AS result;
SELECT test_bms_del_member('(b 42)', 42) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_member('(b 5)', NULL) AS result;
result
--------
-
+ (b 5)
(1 row)
-- bms_del_members()
SELECT test_bms_del_members('(b)', '(b 10)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_members('(b 10)', '(b 10)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_del_members('(b 10)', '(b 5)') AS result;
@@ -262,7 +262,7 @@ SELECT test_bms_del_members('(b 5)', NULL) AS result;
SELECT test_bms_del_members(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
-- bms_join()
@@ -319,7 +319,7 @@ SELECT test_bms_join(NULL, '(b 5)') AS result;
SELECT test_bms_join(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_union()
@@ -341,7 +341,7 @@ SELECT test_bms_union('(b 1 3 5)', '(b)') AS result;
SELECT test_bms_union('(b)', '(b)') AS result;
result
--------
-
+ (b)
(1 row)
-- Overlapping ranges
@@ -383,7 +383,7 @@ SELECT test_bms_union(NULL, '(b 5)') AS result;
SELECT test_bms_union(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_intersect()
@@ -398,14 +398,14 @@ SELECT test_bms_intersect('(b 1 3 5)', '(b 3 5 7)') AS result;
SELECT test_bms_intersect('(b 1 3 5)', '(b 2 4 6)') AS result;
result
--------
-
+ (b)
(1 row)
-- Intersect with empty
SELECT test_bms_intersect('(b 1 3 5)', '(b)') AS result;
result
--------
-
+ (b)
(1 row)
-- Intersect with varrying word counts
@@ -425,25 +425,25 @@ SELECT test_bms_intersect('(b 1 2 3 4 5)', '(b 1 300)') AS result;
SELECT test_bms_intersect('(b 1)', '(b 2)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_intersect('(b 5)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_intersect(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_intersect(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_int_members()
@@ -458,14 +458,14 @@ SELECT test_bms_int_members('(b 1 3 5)', '(b 3 5 7)') AS result;
SELECT test_bms_int_members('(b 1 3 5)', '(b 2 4 6)') AS result;
result
--------
-
+ (b)
(1 row)
-- Intersect with empty
SELECT test_bms_int_members('(b 1 3 5)', '(b)') AS result;
result
--------
-
+ (b)
(1 row)
-- Multiple members
@@ -479,25 +479,25 @@ SELECT test_bms_int_members('(b 0 31 32 63 64)', '(b 31 32 64 65)') AS result;
SELECT test_bms_int_members('(b 1)', '(b 2)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_int_members('(b 5)', NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_int_members(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_int_members(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_difference()
@@ -519,14 +519,14 @@ SELECT test_bms_difference('(b 1 3 5)', '(b 2 4 6)') AS result;
SELECT test_bms_difference('(b 1 3 5)', '(b 1 3 5)') AS result;
result
--------
-
+ (b)
(1 row)
-- Substraction to empty
SELECT test_bms_difference('(b 42)', '(b 42)') AS result;
result
--------
-
+ (b)
(1 row)
-- Subtraction edge case
@@ -556,7 +556,7 @@ SELECT test_bms_difference('(b 1 2 100 200)', '(b 1 2)') AS result;
SELECT test_bms_difference('(b 5)', '(b 5 10)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_difference('(b 5)', NULL) AS result;
@@ -568,13 +568,13 @@ SELECT test_bms_difference('(b 5)', NULL) AS result;
SELECT test_bms_difference(NULL, '(b 5)') AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_difference(NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_is_member()
@@ -887,19 +887,19 @@ SELECT length(test_bms_add_range('(b 1 2)', 200, 250)) AS result;
SELECT test_bms_add_range('(b 5)', 5, NULL) AS result;
result
--------
-
+ (b 5)
(1 row)
SELECT test_bms_add_range('(b 5)', NULL, 10) AS result;
result
--------
-
+ (b 5)
(1 row)
SELECT test_bms_add_range('(b 5)', NULL, NULL) AS result;
result
--------
-
+ (b 5)
(1 row)
SELECT test_bms_add_range(NULL, 5, 10) AS result;
@@ -911,13 +911,13 @@ SELECT test_bms_add_range(NULL, 5, 10) AS result;
SELECT test_bms_add_range(NULL, 10, 5) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_add_range(NULL, NULL, NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_membership()
@@ -1421,7 +1421,7 @@ SELECT test_bms_subset_compare(NULL, NULL) AS result;
SELECT test_bms_copy(NULL) AS result;
result
--------
-
+ (b)
(1 row)
SELECT test_bms_copy('(b 1 3 5 7)') AS result;
@@ -1434,7 +1434,7 @@ SELECT test_bms_copy('(b 1 3 5 7)') AS result;
SELECT test_bms_copy(NULL) AS result;
result
--------
-
+ (b)
(1 row)
-- bms_add_members()
diff --git a/src/test/modules/test_bitmapset/sql/test_bitmapset.sql b/src/test/modules/test_bitmapset/sql/test_bitmapset.sql
index c6e6bce60a3..c7c3cce93d3 100644
--- a/src/test/modules/test_bitmapset/sql/test_bitmapset.sql
+++ b/src/test/modules/test_bitmapset/sql/test_bitmapset.sql
@@ -258,14 +258,13 @@ SELECT test_bms_singleton_member('(b 42)') AS result;
SELECT test_bms_singleton_member(NULL) AS result;
-- bms_get_singleton_member()
-SELECT test_bms_get_singleton_member('(b)', 1000);
+SELECT test_bms_get_singleton_member('(b)');
+-- Try with an empty set
+SELECT test_bms_get_singleton_member(NULL) AS result;
-- Not a singleton, returns input default
-SELECT test_bms_get_singleton_member('(b 3 6)', 1000) AS result;
+SELECT test_bms_get_singleton_member('(b 3 6)') AS result;
-- Singletone, returns sole member
-SELECT test_bms_get_singleton_member('(b 400)', 1000) AS result;
--- Test module checks
-SELECT test_bms_get_singleton_member('', 1000) AS result;
-SELECT test_bms_get_singleton_member(NULL, -1) AS result;
+SELECT test_bms_get_singleton_member('(b 400)') AS result;
-- bms_next_member() and bms_prev_member()
-- First member
diff --git a/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql b/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql
index b95c4d0dda5..227ecb5aa3b 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql
+++ b/src/test/modules/test_bitmapset/test_bitmapset--1.0.sql
@@ -68,7 +68,7 @@ CREATE FUNCTION test_bms_singleton_member(text)
RETURNS integer STRICT
AS 'MODULE_PATHNAME' LANGUAGE C;
-CREATE FUNCTION test_bms_get_singleton_member(text, integer)
+CREATE FUNCTION test_bms_get_singleton_member(text)
RETURNS integer
AS 'MODULE_PATHNAME' LANGUAGE C;
diff --git a/src/test/modules/test_bitmapset/test_bitmapset.c b/src/test/modules/test_bitmapset/test_bitmapset.c
index 2e821320836..88fd71d5495 100644
--- a/src/test/modules/test_bitmapset/test_bitmapset.c
+++ b/src/test/modules/test_bitmapset/test_bitmapset.c
@@ -26,7 +26,6 @@
#include "nodes/pg_list.h"
#include "utils/builtins.h"
#include "utils/timestamp.h"
-#include "varatt.h"
PG_MODULE_MAGIC;
@@ -84,105 +83,109 @@ PG_FUNCTION_INFO_V1(test_random_operations);
#expr, __FILE__, __LINE__); \
} while (0)
+static const char *emptyset = "(b)";
+
/* Encode/Decode to/from TEXT and Bitmapset */
-#define BITMAPSET_TO_TEXT(bms) cstring_to_text(nodeToString(bms))
+#define BITMAPSET_TO_TEXT(bms) cstring_to_text(bms ? nodeToString(bms) : emptyset)
#define TEXT_TO_BITMAPSET(str) ((Bitmapset *) stringToNode(text_to_cstring(str)))
+/* Helper macro to fetch Text parameters as Bitmapsets. SQL-NULL means empty set */
+#define PG_ARG_GETBITMAPSET(n) \
+ (PG_ARGISNULL(n) ? NULL : TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(n)))
+
+/*
+ * Helper macro to handle converting sets back to text, freeing the set and
+ * returning the resulting text representation of the set. Beware of double
+ * evaluation hazard of 'bms'.
+ */
+#define PG_RETURN_BITMAPSET_AS_TEXT(bms) \
+ do { \
+ text *result = BITMAPSET_TO_TEXT(bms); \
+ bms_free(bms); \
+ PG_RETURN_TEXT_P(result); \
+ } while (0);
+
/*
* Individual test functions for each bitmapset API function
+ *
+ * Primarily, we aim to keep these as close to simple wrapper functions as
+ * possible in order to publish the functions of bitmapset.c to the SQL layer
+ * with as little interference as possible. We opt to return SQL NULL in
+ * cases where the input given to the SQL function isn't valid to pass to the
+ * underlying bitmapset.c function. For example we cannot do much useful
+ * testing if someone calls test_bms_make_singleton(NULL) since
+ * bms_make_singleton() expects an integer argument.
+ *
+ * For function arguments which are to be converted to Bitmapsets, we accept
+ * SQL NULL as a valid argument to mean an empty set. Optionally callers may
+ * pass '(b)'.
+ *
+ * For the test functions which return a Bitmapset, these are converted back
+ * to Text with empty sets being represented as '(b)'.
*/
Datum
test_bms_add_member(PG_FUNCTION_ARGS)
{
+ Bitmapset *bms;
int member;
- Bitmapset *bms = NULL;
- text *result;
if (PG_ARGISNULL(1))
- PG_RETURN_NULL();
-
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ PG_RETURN_NULL(); /* invalid input */
+ bms = PG_ARG_GETBITMAPSET(0);
member = PG_GETARG_INT32(1);
- bms = bms_add_member(bms, member);
- result = BITMAPSET_TO_TEXT(bms);
- if (bms)
- bms_free(bms);
+ bms = bms_add_member(bms, member);
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms);
}
Datum
test_bms_add_members(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
- text *result;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
- /* IMPORTANT: bms_add_members modifies/frees the first argument */
+ /* left input is recycled */
bms1 = bms_add_members(bms1, bms2);
+ bms_free(bms2);
- if (bms2)
- bms_free(bms2);
-
- result = BITMAPSET_TO_TEXT(bms1);
- bms_free(bms1);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms1);
}
Datum
test_bms_del_member(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
int32 member;
- text *result;
if (PG_ARGISNULL(1))
- PG_RETURN_NULL();
-
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ PG_RETURN_NULL(); /* invalid input */
+ bms = PG_ARG_GETBITMAPSET(0);
member = PG_GETARG_INT32(1);
- bms = bms_del_member(bms, member);
-
- if (bms_is_empty(bms))
- PG_RETURN_NULL();
- result = BITMAPSET_TO_TEXT(bms);
- bms_free(bms);
+ bms = bms_del_member(bms, member);
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms);
}
Datum
test_bms_is_member(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
int32 member;
bool result;
if (PG_ARGISNULL(1))
- PG_RETURN_BOOL(false);
-
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ PG_RETURN_NULL(); /* invalid input */
+ bms = PG_ARG_GETBITMAPSET(0);
member = PG_GETARG_INT32(1);
- result = bms_is_member(member, bms);
- if (bms)
- bms_free(bms);
+ result = bms_is_member(member, bms);
+ bms_free(bms);
PG_RETURN_BOOL(result);
}
@@ -190,16 +193,11 @@ test_bms_is_member(PG_FUNCTION_ARGS)
Datum
test_bms_num_members(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
- int result = 0;
-
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
+ int result;
result = bms_num_members(bms);
-
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -207,65 +205,40 @@ test_bms_num_members(PG_FUNCTION_ARGS)
Datum
test_bms_make_singleton(PG_FUNCTION_ARGS)
{
- int32 member;
Bitmapset *bms;
- text *result;
+ int32 member;
if (PG_ARGISNULL(0))
- PG_RETURN_NULL();
+ PG_RETURN_NULL(); /* invalid input */
member = PG_GETARG_INT32(0);
bms = bms_make_singleton(member);
- result = BITMAPSET_TO_TEXT(bms);
- bms_free(bms);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms);
}
Datum
test_bms_copy(PG_FUNCTION_ARGS)
{
- text *bms_data;
- Bitmapset *bms = NULL;
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
Bitmapset *copy_bms;
- text *result;
- if (PG_ARGISNULL(0))
- PG_RETURN_NULL();
-
- bms_data = PG_GETARG_TEXT_PP(0);
- bms = TEXT_TO_BITMAPSET(bms_data);
copy_bms = bms_copy(bms);
- result = BITMAPSET_TO_TEXT(copy_bms);
-
- if (bms)
- bms_free(bms);
- if (copy_bms)
- bms_free(copy_bms);
+ bms_free(bms);
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(copy_bms);
}
Datum
test_bms_equal(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
bool result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
result = bms_equal(bms1, bms2);
-
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -273,47 +246,25 @@ test_bms_equal(PG_FUNCTION_ARGS)
Datum
test_bms_union(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
Bitmapset *result_bms;
- text *result;
-
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
result_bms = bms_union(bms1, bms2);
+ bms_free(bms1);
+ bms_free(bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(result_bms);
- bms_free(result_bms);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
}
Datum
test_bms_membership(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
BMS_Membership result;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(BMS_EMPTY_SET);
-
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
result = bms_membership(bms);
-
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32((int32) result);
}
@@ -321,19 +272,18 @@ test_bms_membership(PG_FUNCTION_ARGS)
Datum
test_bms_next_member(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
int32 prevmember;
int result;
- if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
- PG_RETURN_INT32(-2);
+ if (PG_ARGISNULL(1))
+ PG_RETURN_NULL(); /* invalid input */
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ bms = PG_ARG_GETBITMAPSET(0);
prevmember = PG_GETARG_INT32(1);
- result = bms_next_member(bms, prevmember);
- if (bms)
- bms_free(bms);
+ result = bms_next_member(bms, prevmember);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -341,82 +291,41 @@ test_bms_next_member(PG_FUNCTION_ARGS)
Datum
test_bms_intersect(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
Bitmapset *result_bms;
- text *result;
-
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
result_bms = bms_intersect(bms1, bms2);
+ bms_free(bms1);
+ bms_free(bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(result_bms);
- bms_free(result_bms);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
}
Datum
test_bms_difference(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
Bitmapset *result_bms;
- text *result;
-
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
result_bms = bms_difference(bms1, bms2);
+ bms_free(bms1);
+ bms_free(bms2);
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(result_bms);
- bms_free(result_bms);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
}
Datum
test_bms_compare(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
int result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
result = bms_compare(bms1, bms2);
-
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_INT32(result);
}
@@ -424,16 +333,11 @@ test_bms_compare(PG_FUNCTION_ARGS)
Datum
test_bms_is_empty(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
bool result;
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
result = bms_is_empty(bms);
-
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_BOOL(result);
}
@@ -441,22 +345,13 @@ test_bms_is_empty(PG_FUNCTION_ARGS)
Datum
test_bms_is_subset(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
bool result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
result = bms_is_subset(bms1, bms2);
-
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -464,22 +359,13 @@ test_bms_is_subset(PG_FUNCTION_ARGS)
Datum
test_bms_subset_compare(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
BMS_Comparison result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
result = bms_subset_compare(bms1, bms2);
-
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_INT32((int32) result);
}
@@ -487,16 +373,11 @@ test_bms_subset_compare(PG_FUNCTION_ARGS)
Datum
test_bms_singleton_member(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
int result;
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
result = bms_singleton_member(bms);
-
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(result);
}
@@ -504,47 +385,34 @@ test_bms_singleton_member(PG_FUNCTION_ARGS)
Datum
test_bms_get_singleton_member(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
- int32 default_member = PG_GETARG_INT32(1);
- bool success;
- int member = -1;
-
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(default_member);
-
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
+ int member;
/*
- * bms_get_singleton_member returns bool and stores result in member
- * pointer
+ * Keep this simple. Return -1 when we detect the set isn't a singleton
+ * set, otherwise return the singleton member.
*/
- success = bms_get_singleton_member(bms, &member);
- bms_free(bms);
+ if (!bms_get_singleton_member(bms, &member))
+ member = -1;
- if (success)
- PG_RETURN_INT32(member);
+ bms_free(bms);
- PG_RETURN_INT32(default_member);
+ PG_RETURN_INT32(member);
}
Datum
test_bms_prev_member(PG_FUNCTION_ARGS)
{
- text *bms_data;
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
int32 prevmember;
int result;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(-2);
+ if (PG_ARGISNULL(1))
+ PG_RETURN_NULL(); /* invalid input */
- bms_data = PG_GETARG_TEXT_PP(0);
+ bms = PG_ARG_GETBITMAPSET(0);
prevmember = PG_GETARG_INT32(1);
- if (VARSIZE_ANY_EXHDR(bms_data) == 0)
- PG_RETURN_INT32(-2);
-
- bms = TEXT_TO_BITMAPSET(bms_data);
result = bms_prev_member(bms, prevmember);
bms_free(bms);
@@ -554,22 +422,13 @@ test_bms_prev_member(PG_FUNCTION_ARGS)
Datum
test_bms_overlap(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
bool result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
result = bms_overlap(bms1, bms2);
-
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -577,52 +436,47 @@ test_bms_overlap(PG_FUNCTION_ARGS)
Datum
test_bms_overlap_list(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
ArrayType *array;
List *int_list = NIL;
bool result;
- Datum *elem_datums;
- bool *elem_nulls;
+ Datum *elem_datums = NULL;
+ bool *elem_nulls = NULL;
int elem_count;
int i;
- if (PG_ARGISNULL(0))
- PG_RETURN_BOOL(false);
+ bms = PG_ARG_GETBITMAPSET(0);
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (PG_ARGISNULL(1))
+ if (!PG_ARGISNULL(1))
{
- if (bms)
- bms_free(bms);
- PG_RETURN_BOOL(false);
- }
-
- array = PG_GETARG_ARRAYTYPE_P(1);
+ array = PG_GETARG_ARRAYTYPE_P(1);
- deconstruct_array(array,
- INT4OID, sizeof(int32), true, 'i',
- &elem_datums, &elem_nulls, &elem_count);
+ deconstruct_array(array,
+ INT4OID, sizeof(int32), true, 'i',
+ &elem_datums, &elem_nulls, &elem_count);
- for (i = 0; i < elem_count; i++)
- {
- if (!elem_nulls[i])
+ for (i = 0; i < elem_count; i++)
{
- int32 member = DatumGetInt32(elem_datums[i]);
+ if (!elem_nulls[i])
+ {
+ int32 member = DatumGetInt32(elem_datums[i]);
- int_list = lappend_int(int_list, member);
+ int_list = lappend_int(int_list, member);
+ }
}
}
result = bms_overlap_list(bms, int_list);
- if (bms)
- bms_free(bms);
+ bms_free(bms);
list_free(int_list);
- pfree(elem_datums);
- pfree(elem_nulls);
+ if (elem_datums)
+ pfree(elem_datums);
+
+ if (elem_nulls)
+ pfree(elem_nulls);
PG_RETURN_BOOL(result);
}
@@ -630,22 +484,13 @@ test_bms_overlap_list(PG_FUNCTION_ARGS)
Datum
test_bms_nonempty_difference(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
bool result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
result = bms_nonempty_difference(bms1, bms2);
-
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_BOOL(result);
}
@@ -653,22 +498,16 @@ test_bms_nonempty_difference(PG_FUNCTION_ARGS)
Datum
test_bms_member_index(PG_FUNCTION_ARGS)
{
- text *bms_data;
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
int32 member;
int result;
- if (PG_ARGISNULL(0))
- PG_RETURN_INT32(-1);
+ if (PG_ARGISNULL(1))
+ PG_RETURN_NULL(); /* invalid input */
- bms_data = PG_GETARG_TEXT_PP(0);
+ bms = PG_ARG_GETBITMAPSET(0);
member = PG_GETARG_INT32(1);
- if (VARSIZE_ANY_EXHDR(bms_data) == 0)
- PG_RETURN_INT32(-1);
-
- bms = TEXT_TO_BITMAPSET(bms_data);
-
result = bms_member_index(bms, member);
bms_free(bms);
@@ -678,169 +517,85 @@ test_bms_member_index(PG_FUNCTION_ARGS)
Datum
test_bms_add_range(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms;
int32 lower,
upper;
- text *result;
if (PG_ARGISNULL(1) || PG_ARGISNULL(2))
- PG_RETURN_NULL();
-
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
+ PG_RETURN_NULL(); /* invalid input */
+ bms = PG_ARG_GETBITMAPSET(0);
lower = PG_GETARG_INT32(1);
upper = PG_GETARG_INT32(2);
- /* Check for invalid range */
- if (upper < lower)
- {
- if (bms)
- bms_free(bms);
- PG_RETURN_NULL();
- }
-
bms = bms_add_range(bms, lower, upper);
- result = BITMAPSET_TO_TEXT(bms);
- if (bms)
- bms_free(bms);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms);
}
Datum
test_bms_int_members(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
- text *result;
-
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
+ /* left input gets recycled */
bms1 = bms_int_members(bms1, bms2);
- if (bms2)
- bms_free(bms2);
-
- if (bms1 == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(bms1);
-
- if (bms1)
- bms_free(bms1);
+ bms_free(bms2);
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms1);
}
Datum
test_bms_del_members(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
- Bitmapset *result_bms;
- text *result;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
- /* IMPORTANT: bms_del_members modifies/frees the first argument */
- result_bms = bms_del_members(bms1, bms2);
-
- /* bms1 is now invalid, do not free it */
-
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(result_bms);
- bms_free(result_bms);
+ /* left input gets recycled */
+ bms1 = bms_del_members(bms1, bms2);
+ bms_free(bms2);
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms1);
}
Datum
test_bms_replace_members(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
- Bitmapset *result_bms;
- text *result;
-
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
- /* IMPORTANT: bms_replace_members modifies/frees the first argument */
- result_bms = bms_replace_members(bms1, bms2);
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
- /* bms1 is now invalid, do not free it */
+ /* left input gets recycled */
+ bms1 = bms_replace_members(bms1, bms2);
- if (bms2)
- bms_free(bms2);
-
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(result_bms);
- bms_free(result_bms);
-
- PG_RETURN_TEXT_P(result);
+ bms_free(bms2);
+ PG_RETURN_BITMAPSET_AS_TEXT(bms1);
}
Datum
test_bms_join(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
Bitmapset *result_bms;
- text *result;
-
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
- /* IMPORTANT: bms_join may recycle either input arguments */
+ /* either input can be recycled */
result_bms = bms_join(bms1, bms2);
- /* bms1 and bms2 may have been recycled! Do not free any of them. */
+ /* memory cleanup seems more tricky than it's worth here */
- if (result_bms == NULL)
- PG_RETURN_NULL();
-
- result = BITMAPSET_TO_TEXT(result_bms);
- bms_free(result_bms);
-
- PG_RETURN_TEXT_P(result);
+ PG_RETURN_BITMAPSET_AS_TEXT(result_bms);
}
Datum
test_bms_hash_value(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
uint32 hash_result;
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
hash_result = bms_hash_value(bms);
-
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(hash_result);
}
@@ -848,21 +603,12 @@ test_bms_hash_value(PG_FUNCTION_ARGS)
Datum
test_bitmap_hash(PG_FUNCTION_ARGS)
{
- Bitmapset *bms = NULL;
- Bitmapset *bms_ptr;
+ Bitmapset *bms = PG_ARG_GETBITMAPSET(0);
uint32 hash_result;
- if (!PG_ARGISNULL(0))
- bms = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- bms_ptr = bms;
-
/* Call bitmap_hash */
- hash_result = bitmap_hash(&bms_ptr, sizeof(Bitmapset *));
-
- /* Clean up */
- if (!PG_ARGISNULL(0) && bms_ptr)
- bms_free(bms_ptr);
+ hash_result = bitmap_hash(&bms, sizeof(Bitmapset *));
+ bms_free(bms);
PG_RETURN_INT32(hash_result);
}
@@ -870,30 +616,14 @@ test_bitmap_hash(PG_FUNCTION_ARGS)
Datum
test_bitmap_match(PG_FUNCTION_ARGS)
{
- Bitmapset *bms1 = NULL,
- *bms2 = NULL;
- Bitmapset *bms_ptr1,
- *bms_ptr2;
+ Bitmapset *bms1 = PG_ARG_GETBITMAPSET(0);
+ Bitmapset *bms2 = PG_ARG_GETBITMAPSET(1);
int match_result;
- if (!PG_ARGISNULL(0))
- bms1 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(0));
-
- if (!PG_ARGISNULL(1))
- bms2 = TEXT_TO_BITMAPSET(PG_GETARG_TEXT_PP(1));
-
- /* Set up pointers to the Bitmapsets */
- bms_ptr1 = bms1;
- bms_ptr2 = bms2;
-
/* Call bitmap_match with addresses of the Bitmapset pointers */
- match_result = bitmap_match(&bms_ptr1, &bms_ptr2, sizeof(Bitmapset *));
-
- /* Clean up */
- if (bms1)
- bms_free(bms1);
- if (bms2)
- bms_free(bms2);
+ match_result = bitmap_match(&bms1, &bms2, sizeof(Bitmapset *));
+ bms_free(bms1);
+ bms_free(bms2);
PG_RETURN_INT32(match_result);
}
@@ -1031,8 +761,7 @@ test_random_operations(PG_FUNCTION_ARGS)
total_ops++;
}
- if (bms)
- bms_free(bms);
+ bms_free(bms);
PG_RETURN_INT32(total_ops);
}
view thread (81+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add tests for Bitmapset
In-Reply-To: <CAApHDvrvixMmscGEfEyYz0=qbwzM7Y0Up-Uh_AKVmfn8K2sn_A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox