Re: allow frontend use of the backend's core hashing functions
Mark Dilger <mark.dilger@enterprisedb.com>
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Suraj Kharage <suraj.kharage@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2020-02-13T16:26:49Z
Lists: pgsql-hackers
Attachments
- v2-0001-Move-bitmap_hash-and-bitmap_match-to-bitmapset.c.patch (application/octet-stream) patch v2-0001
- v2-0002-Put-all-the-prototypes-for-hashfn.c-into-the-same.patch (application/octet-stream) patch v2-0002
- v2-0003-Adapt-hashfn.c-and-hashutils.h-for-frontend-use.patch (application/octet-stream) patch v2-0003
- v2-0004-Move-src-backend-utils-hash-hashfn.c-to-src-commo.patch (application/octet-stream) patch v2-0004
- (unnamed) (text/plain)
> On Feb 13, 2020, at 3:44 AM, Suraj Kharage <suraj.kharage@enterprisedb.com> wrote: > > 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. +1 > 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); +1 > 3: The first line of the commit message has one typo. > defiend => defined. +1 I have made these changes and rebased Robert’s patches but otherwise changed nothing. Here they are:
Commits
-
Move src/backend/utils/hash/hashfn.c to src/common
- 05d8449e7369 13.0 landed
-
Adapt hashfn.c and hashutils.h for frontend use.
- a91e2fa94180 13.0 landed
-
Put all the prototypes for hashfn.c into the same header file.
- 9341c783cc42 13.0 landed
-
Move bitmap_hash and bitmap_match to bitmapset.c.
- 07b95c3d8334 13.0 landed