Subject: Re: General purpose hashing func in pgbench
To: Teodor Sigaev <teodor@sigaev.ru>, Fabien COELHO <coelho@cri.ensmp.fr>
References: <0e8bd39e-dfcd-2879-f88f-272799ad7ef2@postgrespro.ru>
 <alpine.DEB.2.20.1801092000230.2326@hendaye>
 <63f5a6ac-0c5b-8d59-ca8b-fc8e11244a66@postgrespro.ru>
 <alpine.DEB.2.20.1801101903150.15856@lancre>
 <291822a3-0b3a-0b21-c694-c10776f516ac@postgrespro.ru>
 <6e47c84a-d42a-0e08-1b0d-33e6a6c87778@postgrespro.ru>
 <alpine.DEB.2.20.1801121614470.13422@lancre>
 <bca39e87-6d40-98f6-e4ef-e2d88e109f9c@postgrespro.ru>
 <alpine.DEB.2.20.1801170727240.11884@lancre>
 <960a8096-dde5-5175-7d7c-ef151086cbee@postgrespro.ru>
 <alpine.DEB.2.20.1801180710400.11884@lancre>
 <ae0c93b1-f8a8-a9aa-244c-0c872fa789f0@postgrespro.ru>
 <alpine.DEB.2.20.1801260714070.26762@lancre>
 <12986cad-3dd9-a449-e318-4acd8a8e1697@postgrespro.ru>
 <alpine.DEB.2.20.1801280851260.17596@lancre>
 <b2dd64bb-2d74-bde4-97df-fa1f57f079eb@postgrespro.ru>
 <alpine.DEB.2.20.1801291301500.31442@lancre>
 <736d5e10-8823-a0f8-574e-3ebd7c4a4920@postgrespro.ru>
 <1ad42902-ef1f-a715-7010-1288fb8aae89@sigaev.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
From: Ildar Musin <i.musin@postgrespro.ru>
Message-ID: <75f45845-24ed-347b-66d5-2f18d39c6793@postgrespro.ru>
Date: Tue, 6 Mar 2018 16:47:43 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <1ad42902-ef1f-a715-7010-1288fb8aae89@sigaev.ru>
Content-Type: text/plain; charset=koi8-r; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Teodor,

Thank you for reviewing this patch.

On 06.03.2018 15:53, Teodor Sigaev wrote:
>>> Patch applies, compiles, pgbench & global "make check" ok, doc
>>> built ok.
>
> Agree.
>
> If I understand upthread correctly, implementation of Murmur hash
> algorithm based on Austin Appleby work
> https://github.com/aappleby/smhasher/blob/master/src/MurmurHash2.cpp
>
> If so, I have notice and objections:
>
> 1) Seems, it's good idea to add credits to Austin Appleby to
> comments.
>

Sounds fair, I'll send an updated version soon.

> 2) Reference implementaion directly says (link above): // 2. It will
> not produce the same results on little-endian and big-endian //
> machines.
>
> I don't think that is good thing for testing and benchmarking for
> several reasons: it could produce different data collection,
> different selects, different distribution.
>
> 3) Again, from comments of reference implementation: // Note - This
> code makes a few assumptions about how your machine behaves - // 1.
> We can read a 4-byte value from any address without crashing
>
> It's not true for all supported platforms. Any box with strict
> aligment will SIGBUSed here.
>

I think that both points refer to the fact that original algorithm
accepts a byte string as an input, slices it up by 8 bytes and form
unsigned int values from it. In that case the order of bytes in the
input string does matter since it may result in different integers on
different architectures. And it is also fair requirement for a byte
string to be aligned as some architectures cannot handle unaligned data.
In this patch though I slightly modified the original algorithm in a way
that it takes unsigned ints as an input (not byte string), so neither of
this points should be a problem as it seems to me. But I'll double check
it on big-endian machine with strict alignment (Sparc).

Thanks!

-- 
Ildar Musin
i.musin@postgrespro.ru