Re: BUG #19602: Vuln46: citext split_part silently returns NULL for a zero field position instead of raising core sp

Hi Manu,

Like you, I compared v2 against an exact case-preserving reference on
30000 random mixed-case ASCII inputs (delimiters such as '.', '|', '*'
and 'aB.', positions from -6 to 6 except 0) and found no differences.

EXPLAIN VERBOSE shows that v2 is inlined again. The index scan / seq
scan mismatch after ALTER EXTENSION UPDATE is still there with v2, and
the new paragraph in the commit message covers it.

I have two minor comments:

> +SELECT split_part('aB&cb&d'::citext, 'b&'::citext, -1) = 'd' AS t;

It looks like this test passes even if 'gi' is changed to 'g' in the
function body, since the last field is 'd' either way. With -2, it
would return 'c' only when the match is case-insensitive, so I think
it would catch a missing 'i'. (The same goes for the 'aΔbδc' line in
citext_utf8.sql.)

> +-- (U+01C5) used as the delimiter: the regex engine does not match it
> +-- case-insensitively against itself, but the split is still found

This doesn't seem to hold everywhere. With the builtin provider
(PG_UNICODE_FAST), U+01C5 ~* U+01C5 is false, since allcases() adds
only the lower and upper case forms of a character, not the character
itself. With libc on macOS (en_US.UTF-8), though, it is true, because
towlower() maps U+01C5 to itself, so the 'aDžb' line passes with the
1.8 body as well. Since v2 returns 'b' either way, I think only the
wording needs to change (e.g. "does not match" -> "may not match" in
the comment, and "which happens for" -> "which can happen for" in the
commit message).

Jeff Davis described the same allcases() limitation before, using
'Dž' ~* 'dž' as an example:

https://postgr.es/m/03959b5f8b37b6126d0b9c6ac16c960a94fcd3bb.camel@j-davis.com

Other than that, v2 looks good to me.

Regards,
Jaehyuck Sa