Re: allow frontend use of the backend's core hashing functions - Mailing list pgsql-hackers
| From | Suraj Kharage |
|---|---|
| Subject | Re: allow frontend use of the backend's core hashing functions |
| Date | |
| Msg-id | CAF1DzPXyCi_63j59r5DJCzm99xWfEc9JedmEU_eUj7BkFs5VfA@mail.gmail.com Whole thread Raw |
| In response to | allow frontend use of the backend's core hashing functions (Robert Haas <robertmhaas@gmail.com>) |
| Responses |
Re: allow frontend use of the backend's core hashing functions
|
| List | pgsql-hackers |
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.
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.
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@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=FsGWTA@mail.gmail.com
--
Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.
pgsql-hackers by date: