Horton
Menu

Re: [PATCH] Add tests for Bitmapset

David Rowley <dgrowleyml@gmail.com>

View thread
From: David Rowley <dgrowleyml@gmail.com>
To: Greg Burd <greg@burd.me>
Cc: Michael Paquier <michael@paquier.xyz>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-09-30T23:00:39Z
Lists: pgsql-hackers

Attachments

On Wed, 1 Oct 2025 at 01:38, Greg Burd <greg@burd.me> 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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Minor fixes for test_bitmapset.c

  2. test_bitmapset: Improve random function

  3. Add mem_exceeded_count column to pg_stat_replication_slots.

  4. Tidy-up unneeded NULL parameter checks from SQL function

  5. Remove check for NULL in STRICT function

  6. Fixes for comments in test_bitmapset

  7. Minor fixups of test_bitmapset.c

  8. test_bitmapset: Simplify code of the module

  9. test_bitmapset: Expand more the test coverage

  10. Fix compiler warnings in test_bitmapset

  11. Add a test module for Bitmapset