Date: Tue, 13 Jan 2026 07:27:11 +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: <aWXzzzQcn3hbqG3L@ip-10-97-1-34.eu-west-3.compute.internal>
References: <aS2b3LoUypW1/Gdz@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAGECzQSHb6FxuSdYNY7tt8SKFw2dianACvkExrWRmTBfy-CkjQ@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DFKVK5R7G2ND.1EH6WD16255OW@jeltef.nl>
Archived-At: <https://www.postgresql.org/message-id/aWXzzzQcn3hbqG3L%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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