Hi, I have spent some time reviewing the patches and overall it looks good to me.
However, I have few cosmetic review comments for 0003 patch as below; 1: +++ b/src/backend/utils/hash/hashfn.c @@ -16,15 +16,14 @@ * It is expected that every bit of a hash function's 32-bit result is * as random as every other; failure to ensure this is likely to lead * to poor performance of hash tables. In most cases a hash - * function should use hash_any() or its variant hash_uint32(). + * function should use hash_bytes() or its variant hash_bytes_uint32(), + * or the wrappers hash_any() and *hash_any_uint32* defined in hashfn.h. Here, indicated function name should be *hash_uint32*. 2: I can see renamed functions are declared twice in hashutils.c. I think duplicate declarations after #endif can be removed, +extern uint32 hash_bytes(const unsigned char *k, int keylen); +extern uint64 hash_bytes_extended(const unsigned char *k, + int keylen, uint64 seed); +extern uint32 hash_bytes_uint32(uint32 k); +extern uint64 hash_bytes_uint32_extended(uint32 k, uint64 seed); + +#ifndef FRONTEND .. Wrapper functions .. +#endif + +extern uint32 hash_bytes(const unsigned char *k, int keylen); +extern uint64 hash_bytes_extended(const unsigned char *k, + int keylen, uint64 seed); +extern uint32 hash_bytes_uint32(uint32 k); +extern uint64 hash_bytes_uint32_extended(uint32 k, uint64 seed); 3: The first line of the commit message has one typo. defiend => defined. On Fri, Feb 7, 2020 at 11:00 PM Robert Haas <robertmh...@gmail.com> wrote: > Late last year, I did some work to make it possible to use simplehash > in frontend code.[1] However, a hash table is not much good unless one > also has some hash functions that one can use to hash the keys that > need to be inserted into that hash table. I initially thought that > solving this problem was going to be pretty annoying, but when I > looked at it again today I found what I think is a pretty simple way > to adapt things so that the hashing routines we use in the backend are > easily available to frontend code. > > Here are some patches for that. It may make sense to combine some of > these in terms of actually getting this committed, but I have > separated them here so that it is, hopefully, easy to see what I did > and why I did it. There are three basic problems which are solved by > the three preparatory patches: > > 0001 - hashfn.c has a couple of routines that depend on bitmapsets, > and bitmapset.c is currently backend-only. Fix by moving this code > near related code in bitmapset.c. > > 0002 - some of the prototypes for functions in hashfn.c are in > hsearch.h, mixed with the dynahash stuff, and others are in > hashutils.c. Fix by making hashutils.h the one true header for > hashfn.c. > > 0003 - Some of hashfn.c's routines return Datum, but that's > backend-only. Fix by renaming the functions and changing the return > types to uint32 and uint64, and add static inline wrappers with the > old names that convert to Datum. Just changing the return types of the > existing functions seemed like it would've required a lot more code > churn, and also seems like it could cause silent breakage in the > future. > > Thanks, > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > [1] > http://postgr.es/m/CA+Tgmob8oyh02NrZW=xCScB+5GyJ-jVowE3+TWTUmPF=fsg...@mail.gmail.com > -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
