Re: Safer hash table initialization macro

Hi,

On Tue, Jan 13, 2026 at 10:31:18AM +0100, Jelte Fennema-Nio wrote:
> On Tue Jan 13, 2026 at 8:27 AM CET, Bertrand Drouvot wrote:
> > 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.
> 
> Added a warning in the comment for these macros. For non of our
> usages this was the case (i.e. the char arrays were all storing null
> terminated strings).

Agreed, I did check that too before doing the initial comment.

> So I'm not too worried about this being a problem
> in practice.

I agree, it's very low risk that one adds a new "bad" one in the future. Adding
a comment looks enough then.

+ * WARNING: If you use char[N] to store binary data that is not null-terminated,
+ * the automatic detection will incorrectly treat it as a string and use string
+ * comparison. In such cases, use hash_make_ext() with .force_blobs = true to
+ * override the automatic detection

maybe s/is not null-terminated/may contain null bytes/?

Also, nit, "Note or NOTE" looks more commonly used that "WARNING". We might want
to use that instead.

> Especially because in most cases there will be no null byte
> in the key, and instead you'll start reading out of bounds, which wil
> cause problems pretty much immediately during development.

Agreed.

> Especially, because to make this macro nice to
> use in the two cases that it would apply to we'd have to make it treat 0
> as a special value.

Not necessary, we could also just add a foreach_hash_with_hash_value() to make
things more consistent?

> Finally, I converted the last couple of hash_seq_init stragglers (some
> I had missed/were added) and others needed the now newly added
> foreach_hash_reset macro to be converted.

I see that you added foreach_hash_restart(), I think that makes sense
(even if it's used only in 3 places).

Two more comments:

=== 1

-static void
-cfunc_hashtable_init(void)
-{
-       /* don't allow double-initialization */
-       Assert(cfunc_hashtable == NULL);

Most of the hash_make_fn_cxt() callers check that the destination is already
NULL so that removing the Assert() is not that worrying for them. There are 2
places where it's not the case: InitializeAttoptCache() and build_guc_variables()
, should we add an Assert (or an if check) in them? I think that an Assert is
more appropriate for those 2.

=== 2

"    At the very least we should choose a few places where we use the new
    macros to make sure they have coverage."

I do agree that the refactoring is quite large. I do think there is no rush
to do all the changes at once. We could update a subset of files at a time per
month so that, that would:

- keep changes consistent within each file
- ease the review(s)
- avoid "large" rebases for patches waiting in the commitfest

Thoughts?

Regards,

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