From: | Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Subject: | Re: Bug in huge simplehash |
Date: | 2021-08-13 09:44:17 |
Message-ID: | 5a51d54ad3a8e3cddccc4bd6cfce5dd3@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Drouvot, Bertrand | 2021-08-13 09:45:37 | [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate() |
Previous Message | Andres Freund | 2021-08-13 09:35:46 | Re: Autovacuum on partitioned table (autoanalyze) |