MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Date: Fri, 13 Aug 2021 12:44:17 +0300
From: Yura Sokolov <y.sokolov@postgrespro.ru>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, Tomas Vondra
 <tomas.vondra@2ndquadrant.com>
Subject: Re: Bug in huge simplehash
In-Reply-To: <20210813092133.6it4ztxlz4jncmia@alap3.anarazel.de>
References: <b1f7f32737c3438136f64b26f4852b96@postgrespro.ru>
 <20210813092133.6it4ztxlz4jncmia@alap3.anarazel.de>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <5a51d54ad3a8e3cddccc4bd6cfce5dd3@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/5a51d54ad3a8e3cddccc4bd6cfce5dd3%40postgrespro.ru>
Precedence: bulk

Andres Freund писал 2021-08-13 12:21:
> Hi,
> 
> On 2021-08-10 11:52:59 +0300, Yura Sokolov wrote:
>> - sizemask is set only in SH_COMPUTE_PARAMETERS . And it is set in 
>> this way:
>> 
>>         /* now set size */
>>         tb->size = size;
>> 
>>         if (tb->size == SH_MAX_SIZE)
>>                 tb->sizemask = 0;
>>         else
>>                 tb->sizemask = tb->size - 1;
>> 
>>   that means, when we are resizing to SH_MAX_SIZE, sizemask becomes 
>> zero.
> 
> I think that was intended to be ~0.

I believe so.

>> Ahh... ok, patch is updated to fix this as well.
> 
> 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.

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

> 
> Greetings,
> 
> Andres Freund