Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 3 Jul 2026 16:46:09 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Henson Choi <assam258@gmail.com>
Cc: lex.borisov@gmail.com, hlinnaka@iki.fi, pgsql@j-davis.com,
	pgsql-hackers@postgresql.org
Subject: Re: Improve the performance of Unicode Normalization Forms.
Message-ID: <akdowYMo4GCvjQub@paquier.xyz>
References: <7b76973b-e9b6-4f80-a1d2-4bd467e38346@gmail.com>
 <24338db8-59f2-42a4-a7de-735cf8f12119@gmail.com>
 <af239a46-8869-4ac3-a47f-75abcc0125cb@gmail.com>
 <acN808rQU_mTu-zt@paquier.xyz>
 <fe3bb776-bf6d-40f9-b83a-f64b0948cf6f@gmail.com>
 <acW9Xyld980eorPV@paquier.xyz>
 <f2374ee3-ea9c-490e-9107-4cabd4f026f9@gmail.com>
 <96ed941e-a531-4c4c-a78d-3ba3227f5411@gmail.com>
 <e165c686-a473-4775-9ce2-8a377823e9d6@gmail.com>
 <CAAAe_zDpT1FgfXa4L8hVrkcyugrQ=5PtVT_r8DySCn0akfXEKw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Yxx8i9PTyWL3LlLu"
Content-Disposition: inline
In-Reply-To: 
 <CAAAe_zDpT1FgfXa4L8hVrkcyugrQ=5PtVT_r8DySCn0akfXEKw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/akdowYMo4GCvjQub%40paquier.xyz>
Precedence: bulk


--Yxx8i9PTyWL3LlLu
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Jul 01, 2026 at 03:29:25PM +0900, Henson Choi wrote:
> Two things kept it hidden: no existing test touches this block (so cfbot
> stays green), and even a test that does won't fail without a bounds
> check -- the over-read silently returns 0 and happens to give the right
> answer.
>=20
> Reproduction: the attached test-only patch (nocfbot-unicode-norm-oob.txt)
> adds bounds assertions to the two lookups plus regression tests that hit
> the boundary. On an --enable-cassert build,
>=20
> SELECT normalize(U&'\00C5\+02FA1E', NFD);
> SELECT normalize(U&'\+016D6A\0301', NFC);
>=20
> abort the backend (TRAP: failed Assert("offset + (cp & 63) <
> lengthof(...)")).

That's a good find (if you've used some AI tooling here, good use).  I
don't see an immediate need for in-core checks.  One can also enable
ubsan to get the same effect with out-of-bound lookups reusing your
queries:
=2E./../src/include/common/unicode_norm_table.h:6524:27: runtime error:
index 17438 out of bounds for type 'uint16 [17438]' =20

The tests would be more important.  If we have a gap in these, this
usually points to a first patch where we add more tests.  Please note
that you may not actually need more tests.  If I run the test module
test_saslprep with PG_TEST_EXTRA=3Dsaslprep to make the costly TAP test
run, I get two ubsan reports of this kind.  Note that I enable it in
some of my buildfarm machines, so it would not go unnoticed.

> This extends the tables to 17472/3776, leaves the reader unchanged, and
> resolves those codepoints to the dummy 0 entry (NULL), which is correct.
> The generator change and the regenerated unicode_norm_table.h go
> together. I'd keep the bounds assertion in the lookup as a guard against
> future under-padding.

This kind of issue is bad.  Normalization is applied on the passwords
for SCRAM with SASLprep, meaning this patch would touch code that's
taken before authentication.  This would lead to an immediate CVE, and
most likely one with a bad score.  We need to be extremely careful
with what we are dealing here.  SCRAM is used as default
authentication method for a lot of cloud providers, AFAIK.=20

Saying that (and still trying to figure out the whole code), the
numbers you have published in terms of reduction of the size of the
binaries and the runtimes, based on Jeff's or your methods are
impressive, seeing what has been posted.  I would not underestimate
the cost of the two-pass method we currently use when applying the
normalization (one to calculate the output size, one to do the
normalization).  Hence why don't we just do that first and evaluate
the gain in run time that we get?  One thing that I have been
wondering why reading this thread is simply: why don't we use a=20
BinaryStringInfo and start the normalization with a sensible
preallocated size, avoiding the first pass for the sizing of the
output buffer?  It should be a simpler change that rewriting all the
tables, and I suspect that it may improve the performance quite a bit,
the longer the strings to normalize.  And I also suspect that we
should do that anyway at the end, and do it first.

I am aware of the fact that the patch does some allocation changes,
but I would suggest to split the patch into more reviewable pieces
rather than drop a big chunk of code changes presented as a single
commit, and present one patch for each relevant piece that aims at
improving performance.  Something that shows up as a red flag for me
is the fact that the patch has a long list of bullet points.  Each
bullet point, in some cases, can point to independent patches worth
doing on their own.  Anyway, considering that the simplest pieces may
be better first, I could think of at least:=20
1) Elimination of the second allocation for the recomposition
(in-place recomposition for the decomps buffers).
2) Merge the two-pass decomposition into a single pass
3) The suggestion of Jeff to use a UTF8-native path is also something
I'd wonder about, rather than do the uint32 codepoint dance.  Not sure
about the complexity, but Jeff has mentioned me in the past for his
work on native UTF-8 collation support that relying on the codepoints
in core was much, much, much faster than ICU (that was for sorting,
but we'd burn less CPU for sure here as well).
4) What is now the largest chunk, with the table recreations.  I'd
suggest some more underground work here in terms of tests, identifying
potential gaps in our existing testing that's cheap and good enough
for meaningful corner cases.  I am wondering if we don't have more to
extract here.  One idea: we could store the combining class during
decomposition, saving one extra lookup?

The NF[K]{C,D} operations have been done as they are as a matter of
simplicity, relying on the introduction of the normalization API done
for SCRAM in v10, where performance was not (and is still not)
critical.

As a whole, this patch is too large as-is to be digested in a review.
Due to the pre-authentication requirement, something not rock-solid is
as good as nothing (one can say that for all features, but this one is
particularly exposed).
(Spoiler: I rely on my brain for this kind of work, gentler for the
planet with less tokens and memory burnt.  A patch split as a bunch of
pieces is more edible for me, personally.)
--
Michael

--Yxx8i9PTyWL3LlLu
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmpHaMEACgkQnvQgOdby
QH0N7Q//ULBStdK48LhpgqEw+jY/X15qWUyZXAUTDQW5h8aR1xSBqw4867v5m7HP
U5Vnw5+OV1KWj1EDI8PEnzGtVSHNEnTocC3AzcuJtOO2ubWgruK/Im5mwU8ozAJr
1aObW//jhZTAPOJDH+dL80QKuudGWpkOZkA4ZWKZY3dbkBArnEEFMliIPdlZA0ZO
//2kfzCm/LH4Lj9J+PMETYgRTXZq2au1y2JOk6PI8rhw4IohOQDUdLxOxKVpIOP3
qYI9EuqYPdTXOOwi+Ydykzk8p/EHaeYkV4AVtodenGVV+mlbXOxYkdBg2HIwka1D
CKvpRJ3DjFjH2nqcGZSdahIwF6Cd8YEtilNR/OhbDcU56VayNdqmDoLIVG3BmVBk
pqHtk7Vum7hG161V+ZDMDIEqhR4s3IU0DGaZVOGybP5eH+gf19Xqr+Dmezk7R4mA
1jT19lHQ0hneRf55He7aFXwla4P+4sTSwg6MvwIj+QY44dHwyE0dA60fV6UPTPli
BhfrZ6o8yntmdIv0UdDM16iJ3XpCaFlNq4QrgxyPgjF4rMVJx47KfaGnbCmHcI8I
Fx5A3ILA8Vp20prLNJFZw3M23bfxnSQ9g+l52LsgCsKEnizqfo5xGgyjSnaJosYZ
oipSE7P6pv5+wYpqQBncZt0VBYdf8Jk/B97G1UpCFhhhiLcN1Xk=
=1FQ0
-----END PGP SIGNATURE-----

--Yxx8i9PTyWL3LlLu--