Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.300.41.1.7\))
Subject: Re: Safer hash table initialization macro
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <DFYGONIR99L9.1X97TT8JYZASW@jeltef.nl>
Date: Tue, 27 Jan 2026 15:26:48 +0800
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
 Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <2903820E-A053-4FF8-BEB7-DCECFBB6A21A@gmail.com>
References: <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> <DFY2DVPSXAZX.1BAGDNB27D0F2@jeltef.nl>
 <aXco0XWaces41QFp@ip-10-97-1-34.eu-west-3.compute.internal>
 <DFYFQDUFZ38Q.1LEJWN1P20X8Q@jeltef.nl>
 <aXdBQ+fBKUcqLPzN@ip-10-97-1-34.eu-west-3.compute.internal>
 <DFYGONIR99L9.1X97TT8JYZASW@jeltef.nl>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Archived-At: <https://www.postgresql.org/message-id/2903820E-A053-4FF8-BEB7-DCECFBB6A21A%40gmail.com>
Precedence: bulk


> On Jan 26, 2026, at 18:50, Jelte Fennema-Nio <postgres@jeltef.nl> =
wrote:
>=20
> 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:
>=20
> 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-f=
oreach_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 the an 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 =3D keysize;
+	ctl->entrysize =3D entrysize;
+
+	*flags =3D HASH_ELEM;
+
+	if (opts !=3D NULL && opts->hash !=3D NULL)
+	{
+		/* force_blobs only affects default hash selection, not =
custom hash */
+		Assert(!opts->force_blobs);
+		ctl->hash =3D opts->hash;
+		*flags |=3D HASH_FUNCTION;
+	}
+	else if (opts !=3D NULL && opts->force_blobs)
+	{
+		*flags |=3D HASH_BLOBS;
+	}
+	else
+	{
+		*flags |=3D string_key ? HASH_STRINGS : HASH_BLOBS;
+	}
+
+	if (opts !=3D NULL && opts->match !=3D NULL)
+	{
+		ctl->match =3D opts->match;
+		*flags |=3D HASH_COMPARE;
+	}
```

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

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

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

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 =E2=80=9Cnew=E2=80=9D, =
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=E2=80=99t =
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/