From: Yura Sokolov <y.sokolov@postgrespro.ru>
To: Ranier Vilela <ranier.vf@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Subject: Re: Bug in huge simplehash
Date: Fri, 13 Aug 2021 14:40:08 +0300
Message-ID: <d6493e200932bd231b46d3f8ef7fd493@postgrespro.ru> (raw)
In-Reply-To: <CAEudQAr+pYmPJq6afJboaCWGp-G_Wj=n1kpvX6Sk8M4WkW6uzw@mail.gmail.com>
References: <b1f7f32737c3438136f64b26f4852b96@postgrespro.ru>
<20210813092133.6it4ztxlz4jncmia@alap3.anarazel.de>
<5a51d54ad3a8e3cddccc4bd6cfce5dd3@postgrespro.ru>
<20210813101543.bkncnioyqi472man@alap3.anarazel.de>
<CAEudQAr+pYmPJq6afJboaCWGp-G_Wj=n1kpvX6Sk8M4WkW6uzw@mail.gmail.com>
Ranier Vilela писал 2021-08-13 14:12:
> Em sex., 13 de ago. de 2021 às 07:15, Andres Freund
> <andres@anarazel.de> escreveu:
>
>> Hi,
>>
>> On 2021-08-13 12:44:17 +0300, Yura Sokolov wrote:
>>> Andres Freund писал 2021-08-13 12:21:
>>>> Any chance you'd write a test for simplehash with such huge
>> amount of
>>>> values? It'd require a small bit of trickery to be practical. On
>> systems
>>>> with MAP_NORESERVE it should be feasible.
>>>
>>> Which way C structures should be tested in postgres?
>>> dynahash/simplhash - do they have any tests currently?
>>> I'll do if hint is given.
>>
>> We don't have a great way right now :(. I think the best is to have
>> a
>> SQL callable function that uses some API. See e.g. test_atomic_ops()
>> et
>> al in src/test/regress/regress.c
>>
>>>> > static inline void
>>>> > -SH_COMPUTE_PARAMETERS(SH_TYPE * tb, uint32 newsize)
>>>> > +SH_COMPUTE_PARAMETERS(SH_TYPE * tb, uint64 newsize)
>>>> > {
>>>> > uint64 size;
>>>> >
>>>> > @@ -322,11 +322,7 @@ SH_COMPUTE_PARAMETERS(SH_TYPE * tb,
>> uint32
>>>> > newsize)
>>>> >
>>>> > /* now set size */
>>>> > tb->size = size;
>>>> > -
>>>> > - if (tb->size == SH_MAX_SIZE)
>>>> > - tb->sizemask = 0;
>>>> > - else
>>>> > - tb->sizemask = tb->size - 1;
>>>> > + tb->sizemask = (uint32)(size - 1);
>>>>
>>>> ISTM using ~0 would be nicer here?
>>>
>>> I don't think so.
>>> To be rigid it should be `~(uint32)0`.
>>> But I believe it doesn't differ from `tb->sizemask = (uint32)(size
>> - 1)`
>>> that is landed with patch, therefore why `if` is needed?
>>
>> Personally I find it more obvious to understand the intended
>> behaviour
>> with ~0 (i.e. all bits set) than with a width truncation.
>
> https://godbolt.org/z/57WcjKqMj
> The generated code is identical.
I believe, you mean https://godbolt.org/z/qWzE1ePTE> regards,
> Ranier Vilela
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: y.sokolov@postgrespro.ru, ranier.vf@gmail.com, andres@anarazel.de, pgsql-hackers@lists.postgresql.org, tomas.vondra@2ndquadrant.com
Subject: Re: Bug in huge simplehash
In-Reply-To: <d6493e200932bd231b46d3f8ef7fd493@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox