Thread: simplehash: tb->sizemask = 0

Hi,

I'm a bit puzzled by this code in SH_COMPUTE_PARAMETERS:
   if (tb->size == SH_MAX_SIZE)       tb->sizemask = 0;   else       tb->sizemask = tb->size - 1;

Doesn't that mean that with SH_MAX_SIZE we end up with sizemask being 0
(i.e. no bits set)? At least that's what I get from
   printf("%#x\n", (unsigned int)0);

That would mean SH_INITIAL_BUCKET/SH_NEXT/SH_PREV can only ever return
bucket 0, no?

I don't think we're building hash tables with 2^32 buckets, though.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> I'm a bit puzzled by this code in SH_COMPUTE_PARAMETERS:

>     if (tb->size == SH_MAX_SIZE)
>         tb->sizemask = 0;
>     else
>         tb->sizemask = tb->size - 1;

> Doesn't that mean that with SH_MAX_SIZE we end up with sizemask being 0
> (i.e. no bits set)?

Yeah, which is very obviously broken: for one thing, the Asserts
in SH_NEXT/SH_PREV would surely go off.

(Why are those assertions, anyway, and not test-and-elog?
I do not think an assertion failure is a suitable way to
report "hash table full".)

> I don't think we're building hash tables with 2^32 buckets, though.

What this proves is that nobody has ever tested the behavior at
SH_MAX_SIZE.  I would suggest building a test version with that
set small enough to be conveniently reachable, and then exercising
the behavior as the limit is approached and reached.
        regards, tom lane

On 2017-11-27 22:53:41 -0500, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> > I'm a bit puzzled by this code in SH_COMPUTE_PARAMETERS:
> 
> >     if (tb->size == SH_MAX_SIZE)
> >         tb->sizemask = 0;
> >     else
> >         tb->sizemask = tb->size - 1;
> 
> > Doesn't that mean that with SH_MAX_SIZE we end up with sizemask being 0
> > (i.e. no bits set)?
> 
> Yeah, which is very obviously broken: for one thing, the Asserts
> in SH_NEXT/SH_PREV would surely go off.

That's obviously wrong. Not sure how that happened. I might have had it
as a shift at first?


> (Why are those assertions, anyway, and not test-and-elog?
> I do not think an assertion failure is a suitable way to
> report "hash table full".)

There's a test and elog during insert. Adding actual branches into
SH_NEXT/SH_PREV seems like a bad idea.

Will test a fix.

Greetings,

Andres Freund

On 11/29/17 13:49, Andres Freund wrote:
> On 2017-11-27 22:53:41 -0500, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>>> I'm a bit puzzled by this code in SH_COMPUTE_PARAMETERS:
>>
>>>      if (tb->size == SH_MAX_SIZE)
>>>          tb->sizemask = 0;
>>>      else
>>>          tb->sizemask = tb->size - 1;
>>
>>> Doesn't that mean that with SH_MAX_SIZE we end up with sizemask being 0
>>> (i.e. no bits set)?
>>
>> Yeah, which is very obviously broken: for one thing, the Asserts
>> in SH_NEXT/SH_PREV would surely go off.
> 
> That's obviously wrong. Not sure how that happened. I might have had it
> as a shift at first?
> 
> 
>> (Why are those assertions, anyway, and not test-and-elog?
>> I do not think an assertion failure is a suitable way to
>> report "hash table full".)
> 
> There's a test and elog during insert. Adding actual branches into
> SH_NEXT/SH_PREV seems like a bad idea.
> 
> Will test a fix.

I'll be happy to help test this fix when it's ready.

-- todd