Date: Wed, 14 Jan 2026 07:57:29 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Safer hash table initialization macro
Message-ID: <aWdMaa/3m1n9wcX5@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aTAcV/OF0uAfT4rL@ip-10-97-1-34.eu-west-3.compute.internal>
 <DEPJFNNOXN6C.39PM8C3I63DKU@jeltef.nl>
 <CA+hUKG+TuQPXtfkaam6trfLrk+OXf89zUt7Jx27zEY-y8i1swA@mail.gmail.com>
 <CAGECzQSnhom3wFnY6a7xepGfxzzVmwyNCcJQE=y8UYS-g2G=RQ@mail.gmail.com>
 <CA+hUKGLZPZ4DgJs46-dkkkF5pi0gm23SerJY0HbOUP5r7siPsg@mail.gmail.com>
 <DESS1U66SWHO.2JT1YL2Q20K0F@jeltef.nl>
 <aTfPRxNYuYWVLI+3@ip-10-97-1-34.eu-west-3.compute.internal>
 <DFKVK5R7G2ND.1EH6WD16255OW@jeltef.nl>
 <aWXzzzQcn3hbqG3L@ip-10-97-1-34.eu-west-3.compute.internal>
 <DFNCUVLOL3E1.37T3ZSNLVDW42@jeltef.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DFNCUVLOL3E1.37T3ZSNLVDW42@jeltef.nl>
Archived-At: <https://www.postgresql.org/message-id/aWdMaa/3m1n9wcX5%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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