From: | Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> |
---|---|
To: | Suraj Kharage <suraj(dot)kharage(at)enterprisedb(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: allow frontend use of the backend's core hashing functions |
Date: | 2020-02-13 16:26:49 |
Message-ID: | 9FD2663D-BDA1-4BB2-9ECC-77545B816760@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On Feb 13, 2020, at 3:44 AM, Suraj Kharage <suraj(dot)kharage(at)enterprisedb(dot)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:
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Move-bitmap_hash-and-bitmap_match-to-bitmapset.c.patch | application/octet-stream | 3.8 KB |
v2-0002-Put-all-the-prototypes-for-hashfn.c-into-the-same.patch | application/octet-stream | 3.4 KB |
v2-0003-Adapt-hashfn.c-and-hashutils.h-for-frontend-use.patch | application/octet-stream | 7.0 KB |
v2-0004-Move-src-backend-utils-hash-hashfn.c-to-src-commo.patch | application/octet-stream | 21.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-02-13 16:30:45 | Re: In PG12, query with float calculations is slower than PG11 |
Previous Message | Tom Lane | 2020-02-13 16:26:45 | Re: Small docs bugfix: make it clear what can be used in UPDATE FROM and DELETE USING |