Re: [PATCH] Add tests for Bitmapset

On Wed, Oct 01, 2025 at 12:00:39PM +1300, David Rowley wrote:
> 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.

+#define PG_RETURN_BITMAPSET_AS_TEXT(bms) \
+   do { \
+       text       *result = BITMAPSET_TO_TEXT(bms); \
+       bms_free(bms); \
+       PG_RETURN_TEXT_P(result); \
+   } while (0);

This is also something I've considered as embedding in a macro, and
discarded it.  The reason was code readability, because it gives to
the reader a quick knowledge of what can be freed once a call to a
specific bitmapset.c routine is done, acting as a kind of reference
template.  Here, the free call knowledge gets hidden.  test_bms_copy()
becomes more confusing to me.  This argument comes down to one's taste
and philosophy on the matter of test modules.  These are for me tools
of coverage, but also can work as reference templates when calling an
internal routine because we also document some of its implied
behaviors, like the inputs recycled.

> 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.

+static const char *emptyset = "(b)";

Not sure that I'm on board with hardcoding this knowledge in the test
module.  But well, if you feel strongly about it, I'm not going to put
a fight for it.

> 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.

It is not very far.  In short, I am caring about the status returned
by bms_get_singleton_member(), then translated in the output of the
SQL function based on the default given in input:
https://www.postgresql.org/message-id/aNolInqSzt1d1338@paquier.xyz

Perhaps not the most elegant solution, but simpler than returning a
setof without changing the meaning of the tests.  So we could return
the status directly or use the default value from the input, not doing
any of these actions reduces what's reported back at SQL level.

> 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.

+/* 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)))

True that embedding the PG_ARGISNULL() call may be better in a macro,
as passing NULL values to the internal routines is OK.

> 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().

I've wondered about that as well.  However, I have concluded to leave
the free calls arounds, for the memory context argument if someone
plays more with this module.  I don't mind the removal of the NULL
checks when calling bms_free() to cut lines in the module, though.  I
was also seeing an argument with cross-checking the frees after a
REALLOCATE_BITMAPSETS copy, as well, for sanity checks.  So I would
rather leave them as they are, not remove them.  Again, that may come
up to one's view on the matter and one's philosophy around these
modules.
--
Michael