public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bertrand Drouvot <[email protected]>
To: Jelte Fennema-Nio <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: [email protected]
Subject: Re: Safer hash table initialization macro
Date: Tue, 13 Jan 2026 07:27:11 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <aS2b3LoUypW1/[email protected]>
	<CAGECzQSHb6FxuSdYNY7tt8SKFw2dianACvkExrWRmTBfy-CkjQ@mail.gmail.com>
	<aTAcV/[email protected]>
	<[email protected]>
	<CA+hUKG+TuQPXtfkaam6trfLrk+OXf89zUt7Jx27zEY-y8i1swA@mail.gmail.com>
	<CAGECzQSnhom3wFnY6a7xepGfxzzVmwyNCcJQE=y8UYS-g2G=RQ@mail.gmail.com>
	<CA+hUKGLZPZ4DgJs46-dkkkF5pi0gm23SerJY0HbOUP5r7siPsg@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>

Hi,

On Sat, Jan 10, 2026 at 12:32:39PM +0100, Jelte Fennema-Nio wrote:
> On Tue Dec 9, 2025 at 8:27 AM CET, Bertrand Drouvot wrote:
> > Thanks for this patch series!
> 
> v4 attached, which fixes some rebase conflicts. Also I moved the patches
> that start using the new API to the end of the series, so the
> introduction of the new APIs is now done in the first two patches.

Thanks for the new version!

A few random comments:

=== 1

+#define HASH_PTR_AS_STRING(ptr, size) \
+       (pg_expr_has_type_p((ptr), char(*)[size]) || pg_expr_has_type_p((ptr), NameData *))
+#define HASH_KEY_AS_STRING(entrytype, keymember) \
+       HASH_PTR_AS_STRING(&((entrytype *)0)->keymember, \
+                                          sizeof(((entrytype *)0)->keymember))
+#define HASH_TYPE_AS_STRING(type) \
+       HASH_PTR_AS_STRING(pg_nullptr_of(type), sizeof(type))


I've probably a too paranoid concern: what if someone use char[N] to store
binary stuff with embedded null? That would detect it as string and then
make use of strncmp() and then would provide incorrect result.

While the risk is probably very low, I think it is there.

What about using:

hash_make_str()
hash_make_blob()

or force a flag:

hash_make(..., HASH_STRINGS)
hash_make(..., HASH_BLOBS)

instead? That breaks some of the patch's intend, so I'm not sure it's
worth it given the low probability risk...

=== 2

The patch introduces foreach_hash() but the foreach_hash_with_hash_value()
counterpart seems missing. It could be used in TypeCacheTypCallback() and
InvalidateAttoptCacheCallback().

I think that we could add it or make the new hash_seq_new() accept an extra
hashvalue parameter? (when set to 0 would call hash_seq_init() and
hash_seq_init_with_hash_value() otherwise?

Some minor comments:

=== 3

+extern "C++" {

should be "+extern "C++"
+{"

as per pgindent.

=== 4

+#define pg_nullptr_of(type) ((type *)NULL)

s/(type *)NULL/(type *) NULL/ ? (and in the comment in top of it)

=== 5

+#define foreach_hash(type, var, htab) \
+       for (type *var = 0, *var##__outerloop = (type *) 1; \

s/type *var = 0/type *var = NULL/? (see ec782f56b0c)

=== 6

+           * serialized++ = *value;

s/* serialized/*serialized

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Safer hash table initialization macro
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox