Feedback-ID: i675e48f3:Fastmail
Date: Wed, 17 Sep 2025 07:59:15 -0400
From: Greg Burd <greg@burd.me>
To: Michael Paquier <michael@paquier.xyz>, Masahiko Sawada
 <sawada.mshk@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Nathan
 Bossart <nathandbossart@gmail.com>, Robert Haas
 <robertmhaas@gmail.com>
Message-ID: <691D5239-9A9F-4537-A75D-2045BFF66712@greg.burd.me>
In-Reply-To: <aMoCUogZMamHRJad@paquier.xyz>
References: <aMoCUogZMamHRJad@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/691D5239-9A9F-4537-A75D-2045BFF66712%40greg.burd.me>
Precedence: bulk


On Sep 16 2025, at 8:35 pm, Michael Paquier <michael@paquier.xyz> wrote:

> On Tue, Sep 16, 2025 at 03:00:40PM -0700, Masahiko Sawada wrote:
>> Thank you for updating the patch. It seems cfbot caught a regression
>> test error[1] in a 32-bit build.
> 
> - bitmap_hash [1,3,5] |   49870778
> + bitmap_hash [1,3,5] | 1509752520
> 
> This one is able the hash value computation being not portable across
> architectures.  I would just change these to be non-NULL, I guess.
> --
> Michael

Thanks Michael, Sawada-san, yep hashing is arch dependent (go figure!),
but thankfully it is deterministic. Rather that test for non-NULL I've
come up with this approach:

-- Architecture-aware hash testing
WITH arch_info AS (
    SELECT
        CASE
            WHEN pg_column_size(1::bigint) = 8 THEN '64bit'
            ELSE '32bit'
        END as architecture
),
expected_values AS (
    SELECT
        architecture,
        CASE architecture
            WHEN '64bit' THEN 0
            WHEN '32bit' THEN 0
        END as hash_null,
        CASE architecture
            WHEN '64bit' THEN 49870778
            WHEN '32bit' THEN 1509752520
        END as hash_135,
        CASE architecture
            WHEN '64bit' THEN -303921606
            WHEN '32bit' THEN 0  -- TBD
        END as hash_246
    FROM arch_info
)
SELECT 'expected hash NULL' as test,
       test_bitmap_hash(NULL) = hash_null as result
FROM expected_values
UNION ALL
SELECT 'expected hash [1,3,5]' as test,
       test_bitmap_hash(test_bms_from_array(ARRAY[1,3,5])) = hash_135 as result
FROM expected_values
UNION ALL
SELECT 'expected hash [2,4,6]' as test,
       test_bitmap_hash(test_bms_from_array(ARRAY[2,4,6])) = hash_246 as result
FROM expected_values;

However I'm not sure what the value is for testing hash functions except
for coverage and I'm fairly certain that function is well exercised. 
That said, I think that will work.  I'll let cfbot tell me the 32bit
hash value on the next run.

thanks again for the helpful insights,

-greg