Feedback-ID: i675e48f3:Fastmail
Date: Mon, 22 Sep 2025 09:30:24 -0400
From: Greg Burd <greg@burd.me>
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Robert Haas
 <robertmhaas@gmail.com>, Nathan Bossart <nathandbossart@gmail.com>,
 Masahiko Sawada <sawada.mshk@gmail.com>
Message-ID: <BC5ECB07-A9A3-44CC-848E-B742593B438A@greg.burd.me>
In-Reply-To: <aNEBUeYqqSVOSVaP@paquier.xyz>
References: <aNEBUeYqqSVOSVaP@paquier.xyz>
Subject: Re: [PATCH] Add tests for Bitmapset
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Archived-At: 
 <https://www.postgresql.org/message-id/BC5ECB07-A9A3-44CC-848E-B742593B438A%40greg.burd.me>
Precedence: bulk


On Sep 22 2025, at 3:57 am, Michael Paquier <michael@paquier.xyz> wrote:

> On Fri, Sep 19, 2025 at 02:48:43PM -0400, Greg Burd wrote:
>> I think I've incorporated your feedback which did indeed make the patch
>> more readable/crisp and maintainable, thank you.
>> 
>> coverage:    HEAD    v7    v8
>> lines......:  87.1  90.5  92.2

Michael,

Thank you for your time and help.

> bitmap_match() had some duplicated tests.
> 
> bms_replace_members() was not covered, and a function was added to the
> module.  I get up to 93.4% for the lines, once added.
> 
> +PG_FUNCTION_INFO_V1(test_bms_from_array);
> +PG_FUNCTION_INFO_V1(test_bms_to_array);
> These two were still declared, are not required.
> 
> There were a couple of functions that acted as dead code in the
> module, covered in the tree.  As we are trying to cover the gap I
> think that we should just do the whole thing.  Here are the functions: 
> - is_empty
> - subset_compare
> - get_singleton_member
> - nonempty_difference
> - member_index
> - join.  The test C function had a mistake, because either input may
> be modified or updated by bms_join().  The code was freeing the right
> input, leading to errors in the node output function when tested.

All great ideas, apologies for the oversights.

> Mixing the CTEs and the UNION ALL would have been nice if the queries
> in the UNIONs relied on more than one input value, but none of them
> did that, so I have removed this part and made the tests leaner.  That
> feels easier for the eye.

Agreed, thank you.

> Testing on equality for the hash functions should be OK, so I have
> kept these, including the NULL/0 cases.

Sounds good to me.

> A few incorrect things related to the style, fixed on the way, like an
> incorrect copyright notice in meson.build, the top of test_bitmapset.c
> was largely incorrect.  It seems like these were copy-pasted from
> elsewhere.

Yes, copied from the radixtree test module.

> Another thing was testing with -DREALLOCATE_BITMAPSETS, which I've
> done.

That seems like a good idea, I thought about doing that as well.

> A couple of code paths related to the test C functions don't
> test for NULL input, which is still OK as these don't impact the
> internal tests or the backend coverage, so I've left these out.

Fair.

> The result I had was good enough, so applied.  The CI was OK, the
> buildfarm may have a different opinion.
> --
> Michael

Thanks for the help getting this done, I really appreciate it.  So far
the buildfarm seems okay. :)

best.

-greg