Re: allow frontend use of the backend's core hashing functions

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 <robertmhaas(at)gmail(dot)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=FsGWTA@mail.gmail.com
>

--
--

Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.