MIME-Version: 1.0
References: 
 <CAApHDvq-4994jpDnspQ+bgSbzo0+=Z4YQbj+YfP7wi97B0e1qg@mail.gmail.com>
 <76DBEF5D-EA47-4C30-8D28-97B3B88E2E2D@greg.burd.me>
 <CAApHDvrvixMmscGEfEyYz0=qbwzM7Y0Up-Uh_AKVmfn8K2sn_A@mail.gmail.com>
 <aNxpxE6a9G3sbfrm@paquier.xyz>
In-Reply-To: <aNxpxE6a9G3sbfrm@paquier.xyz>
From: David Rowley <dgrowleyml@gmail.com>
Date: Wed, 1 Oct 2025 13:07:10 +1300
Message-ID: 
 <CAApHDvoC-W3s2QJdBY6B32BXy2br9iGsNJ3v4uLFOosBgmY2YA@mail.gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
To: Michael Paquier <michael@paquier.xyz>
Cc: Greg Burd <greg@burd.me>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvoC-W3s2QJdBY6B32BXy2br9iGsNJ3v4uLFOosBgmY2YA%40mail.gmail.com>
Precedence: bulk

On Wed, 1 Oct 2025 at 12:37, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

This sounds like another argument towards getting rid of the bms_frees
from the module.

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

I'm not a huge fan of that part either. I didn't feel strongly enough
to change it when adjusting Greg's v2. For me, I don't see the problem
with leaving it up to nodeToString(), which will return "<>" for an
empty set.

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

Thanks for explaining. Is anything being lost with the -1 return?
Since that's not a valid member for a set, -1 can only be returned if
bms_get_singleton_member() returned false.

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

OK good. There's a decent code reduction from this one.

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

Because you're not happy with embedding the bms_free() in with
PG_RETURN_BITMAPSET_AS_TEXT(), why don't we just get rid of all the
bms_free's?  It's just a test module. If anyone ever comes complaining
about memory usage then we can reconsider. I think it's nice to keep
these wrappers as thin as possible in terms of the amount of C code.
Having conditional logic in them, IMO, risk hiding the true behaviour
of the underlying code that's being tested.

I know I've just come in here like a bull in a china shop. It's not my
intention to imply that anything is wrong with the code. I just put
together my thoughts in the hopes that it can be made even better.

David