Date: Wed, 14 Jan 2026 09:28:37 +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: <aWdhxU/T+PoNB9j6@ip-10-97-1-34.eu-west-3.compute.internal>
References: <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>
 <aWdMaa/3m1n9wcX5@ip-10-97-1-34.eu-west-3.compute.internal>
 <DFO6J99E4HOY.29NFF8O6W6TFH@jeltef.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DFO6J99E4HOY.29NFF8O6W6TFH@jeltef.nl>
Archived-At: <https://www.postgresql.org/message-id/aWdhxU/T%2BPoNB9j6%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Wed, Jan 14, 2026 at 09:46:41AM +0100, Jelte Fennema-Nio wrote:
> On Wed, 14 Jan 2026 at 08:57, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
> > + * 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/?
> 
> Changed wording and changed to NOTE.

Thanks, looks good.

> So if we don't have the foreach_hash_with_hash_value macro treat 0
> special, then we'd need to replace this single while with two for
> foreach loops. A foreach_hash for the 0 case and a
> foreach_hash_with_hash_value for the non-zero one.

Okay, let's forget about introducing foreach_hash_with_hash_value() then.

> > -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.
> 
> I'm a bit confused about this comment. I didn't change anything for
> those two places about their checks for NULL. Did you mean this as a
> separate but related improvement to existing code?

Agree that you didn't change for NULL check in those places.
I meant that 0003 introduced calling hash_make_fn_cxt() in InitializeAttoptCache()
and build_guc_variables(), which made me think if we could add an Assert while
at it.

> (To be clear, the removed Assert that you quoted doesn't matter when
> inlining, because it's the only item in an if (!cfunc_hashtable) block)

Right, the removed Assert is fine.

> 
> As long as the new macro definitions get in, any way a committer thinks
> that's sensible is fine by me. e.g. The List foreach macros also haven't
> been replaced in bulk, but I'm still very happy that I can use them for
> new code.

+1

Regards,

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