| From: | Andres Freund <andres(at)anarazel(dot)de> |
|---|---|
| To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
| Cc: | Álvaro Herrera <alvherre(at)kurilemu(dot)de>, "Masashi Kamura (Fujitsu)" <kamura(dot)masashi(at)fujitsu(dot)com>, "'pgsql-hackers(at)lists(dot)postgresql(dot)org'" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Jeff Davis <pgsql(at)j-davis(dot)com> |
| Subject: | Re: Crash issue in PG18.5 regression |
| Date: | 2026-08-11 18:31:22 |
| Message-ID: | v3nniwcrxejmcfvz56xbd22hphprqleuornd6hqkmw2bl7kgmz@cnytz2ee5ltk |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On 2026-08-11 12:41:49 +0300, Heikki Linnakangas wrote:
> +/* lowercasing/casefolding in C locale */
> +static size_t
> +strlower_c(char *dst, size_t dstsize, const char *src, size_t srclen)
> +{
> + int i;
> +
> + for (i = 0; i < srclen && i < dstsize; i++)
> + dst[i] = pg_ascii_tolower(src[i]);
> + if (i < dstsize)
> + dst[i] = '\0';
> + return srclen;
> +}
Hm. If I infer the pg_strlower() API correctly - it's utterly underdocumented
- it seems to be inteded to support a few things in 18:
1) srclen = -1 works
Inferred from unicode_strlower()'s comment:
* String src must be encoded in UTF-8. If srclen < 0, src must be
* NUL-terminated.
Also note that srclen is ssize_t. This changed in 6d22c67c3bf5 (recently).
2) The required length for the conversion is returned, even if the destination
is too short (including when dstlen = 0)
* Result string is stored in dst, truncating if larger than dstsize. If
* dstsize is greater than the result length, dst will be NUL-terminated;
* otherwise not.
*
* If dstsize is zero, dst may be NULL. This is useful for calculating the
* required buffer size before allocating.
It's really a guessing game though, due to the religious under documentation
of the generic functions. Why does unicode_strlower() have docs, but
pg_strlower() does not?
Both don't seem quite right given this implementation.
I don't quite know whether we need to fix these, given the lack of problematic
uses in tree, the time pressure, but it also seems like a recipe for future
disaster to leave it like this.
I'd also make i size_t, given that the input is size_t. Perhaps practically
no problem, but I see no reason to not use size_t here.
It also seems like we really ought to have an actually reachable, currently
crashing, to_date() call in the tests? It seems concerning that
seq_search_localized(), casefold_str_cmp() are completely uncovered today, and
quite obviously we can't be relied upon to get this right.
https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html#L2379
Greetings,
Andres Freund
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andres Freund | 2026-08-11 18:47:11 | Re: [PATCH] bufmgr: tighten LWLock:BufferMapping on InvalidateBuffer |
| Previous Message | Alexandre Felipe | 2026-08-11 18:27:37 | Re: Restructured Shared Buffer Hash Table |