Обсуждение: 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