Re: Safer hash table initialization macro - Mailing list pgsql-hackers

> On Jan 26, 2026, at 18:50, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Mon Jan 26, 2026 at 11:26 AM CET, Bertrand Drouvot wrote:
>> I can still see it. If I apply from 0001 to 0004 and compile, I see it. It looks
>> like it's fixed in 0005:
>
> Ugh, you're right. I had ammended that fix to the wrong commit. Fixed
> now.
>
<v9-0001-Add-hash_make-macros.patch><v9-0002-Add-foreach_hash-macro.patch><v9-0003-Use-hash_make-macros-throughout-the-codebase.patch><v9-0004-Use-foreach_hash-macro-throughout-the-codebase.patch><v9-0005-Inline-functions-that-have-now-become-trivial.patch>

Hi Jelte,

A solid patch. Looks like 0004 is conflict to master branch (3fccbd94cba), thus needs a rebase. Anyway, I hard reset
thean early commit and applied the patch set locally. 

Here are a few comments for 0001 and 0002:

1 - 0001
```
void
+hash_opts_init(HASHCTL *ctl, int *flags,
+               Size keysize, Size entrysize, bool string_key,
+               const HASHOPTS *opts)
+{
+    MemSet(ctl, 0, sizeof(*ctl));
+    ctl->keysize = keysize;
+    ctl->entrysize = entrysize;
+
+    *flags = HASH_ELEM;
+
+    if (opts != NULL && opts->hash != NULL)
+    {
+        /* force_blobs only affects default hash selection, not custom hash */
+        Assert(!opts->force_blobs);
+        ctl->hash = opts->hash;
+        *flags |= HASH_FUNCTION;
+    }
+    else if (opts != NULL && opts->force_blobs)
+    {
+        *flags |= HASH_BLOBS;
+    }
+    else
+    {
+        *flags |= string_key ? HASH_STRINGS : HASH_BLOBS;
+    }
+
+    if (opts != NULL && opts->match != NULL)
+    {
+        ctl->match = opts->match;
+        *flags |= HASH_COMPARE;
+    }
```

This function has a lot of duplicate checks on opts!=NULL, I think it can be simplified as:
```
    *flags = HASH_ELEM;

    if (opts == NULL)
    {
        *flags |= string_key ? HASH_STRINGS : HASH_BLOBS;
        return;
    }

    if (opts->hash != NULL)
    {
        /* force_blobs only affects default hash selection */
        Assert(!opts->force_blobs);
        ctl->hash = opts->hash;
        *flags |= HASH_FUNCTION;
    }
    else if (opts->force_blobs)
    {
        *flags |= HASH_BLOBS;
    }
    ….
```

2 - 0002
```
+HASH_SEQ_STATUS
+hash_seq_new(HTAB *hashp)
+{
+    HASH_SEQ_STATUS status;
+
+    hash_seq_init(&status, hashp);
+    return status;
+}
```

Why this function returns a structure by value? Which looks quite uncommon. Usually, when a function is named with
“new”,it returns a pointer to a new object. 

3 - 0002

foreach_hash feels fragile. It requires to call foreach_hash_term before break, which is easy to forget. And the
documentation   doesn’t mention how to continue, how to return from a loop, and how to goto from inside a loop. 

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/