I'll analyze this patch carefully looking for behavioral changes and transcription errors. Let me examine each section systematically.Let me walk through the key areas of concern: ## Issue 1: Loss of bounds checking in `regc_pg_locale.c` (likely a real bug) In the original code for `pg_wc_isdigit` (and all the other `pg_wc_is*` functions), the LIBC paths had explicit bounds checks: **Original `LIBC_WIDE` (UTF8):** ```c if (sizeof(wchar_t) >= 4 || c <= (pg_wchar) 0xFFFF) return iswdigit_l((wint_t) c, pg_regex_locale->info.lt); /* FALL THRU */ ``` **Original `LIBC_1BYTE`:** ```c return (c <= (pg_wchar) UCHAR_MAX && isdigit_l((unsigned char) c, pg_regex_locale->info.lt)); ``` In the new code, these become unconditional calls like `wc_isdigit_libc_sb` which just does: ```c return isdigit_l((unsigned char) wc, locale->info.lt); ``` The cast `(unsigned char) wc` truncates `wc` silently. So if `c > UCHAR_MAX`, the old code returned 0 (false); the new code calls `isdigit_l` on the truncated byte, which could return true for, say, `c = 0x100 + '0'`. **However**, this is mitigated by the `max_chr` field. For `ctype_methods_libc_sb` and `ctype_methods_libc_other_mb`, `max_chr = UCHAR_MAX`, and `pg_ctype_get_cache` uses this to limit the range explored. So in regex character class caching, the new code only invokes these methods for `c <= UCHAR_MAX`. But the `pg_wc_is*` functions are also called directly from `pg_regc_locale.c` outside the cache path (e.g., for testing specific characters during regex matching). Looking at the original code's `LIBC_1BYTE` branch, it explicitly guarded `c <= UCHAR_MAX`. The new code doesn't. **This is a behavioral regression for single-byte LIBC**: a `pg_wchar` value above 255 will now silently truncate and yield a possibly-wrong result rather than returning 0. Similarly for `LIBC_WIDE` (UTF8 path): the old code had a `sizeof(wchar_t) >= 4 || c <= 0xFFFF` guard, falling through to `LIBC_1BYTE` (with its `<= UCHAR_MAX` check) when wchar_t is 16-bit and `c > 0xFFFF`. The new `wc_isdigit_libc_mb` unconditionally does `iswdigit_l((wint_t) wc, ...)`, which on 16-bit-wchar_t platforms (Windows) truncates `wc` to 16 bits. The old behavior on such platforms was to fall through to the single-byte path, which then returned 0 for `c > UCHAR_MAX`. **New behavior on 16-bit wchar_t platforms differs from old.** ## Issue 2: `toupper_libc_mb` / `tolower_libc_mb` on 16-bit wchar_t platforms Original code: ```c case PG_REGEX_STRATEGY_LIBC_WIDE: if (pg_regex_locale->is_default && c <= 127) ... if (sizeof(wchar_t) >= 4 || c <= 0xFFFF) return towupper_l(...); /* FALL THRU */ case PG_REGEX_STRATEGY_LIBC_1BYTE: if (pg_regex_locale->is_default && c <= 127) ... if (c <= UCHAR_MAX) return toupper_l(...); return c; ``` New `toupper_libc_mb`: ```c if (sizeof(wchar_t) >= 4 || wc <= (pg_wchar) 0xFFFF) return towupper_l((wint_t) wc, locale->info.lt); else return wc; ``` The fall-through on 16-bit wchar_t platforms is lost. The old code, when `sizeof(wchar_t) < 4` and `c > 0xFFFF`, would fall through and try the single-byte path. New code just returns `wc` unchanged. This is a behavioral change on Windows/16-bit-wchar_t platforms for characters above 0xFFFF — but in UTF8 the old fall-through to `toupper_l` was nearly useless anyway (since the byte truncation is meaningless), so this is arguably a fix. Still worth flagging. ## Issue 3: `char_is_cased_libc` — behavioral change for libc, single-byte, high-bit set In `like_support.c`, original code: ```c else if (is_multibyte && IS_HIGHBIT_SET(c)) return true; else if (locale->provider != COLLPROVIDER_LIBC) return IS_HIGHBIT_SET(c) || (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'); else return isalpha_l((unsigned char) c, locale->info.lt); ``` For **libc, single-byte encoding**, with high-bit-set char: original called `isalpha_l`. New `char_is_cased_libc`: ```c if (is_multibyte && IS_HIGHBIT_SET(ch)) return true; else return isalpha_l((unsigned char) ch, locale->info.lt); ``` For libc + single-byte + high-bit-set, both old and new call `isalpha_l`. ✓ Matches. For libc + multibyte + high-bit-set: both return true. ✓ Matches. For libc + ASCII letter: both call `isalpha_l`. ✓ Matches. For libc + ASCII non-letter: both call `isalpha_l` → false. ✓ Matches. OK, that one is fine. ## Issue 4: `Generic_Text_IC_like` semantic change Original: ```c if (pg_database_encoding_max_length() > 1 || (locale->provider == COLLPROVIDER_ICU)) /* call lower() then match */ else /* SB_IMatchText fast path */ ``` New: ```c if (locale->ctype_is_c || (char_tolower_enabled(locale) && pg_database_encoding_max_length() == 1)) /* SB_IMatchText fast path */ else /* call lower() then match */ ``` Old fast-path condition: `max_length == 1 && provider != ICU` — meaning libc or builtin with single-byte encoding. New fast-path condition: `ctype_is_c || (char_tolower_enabled && max_length == 1)`. - `char_tolower` is defined only for libc (the `ctype_methods_libc_*` tables set `.char_tolower = char_tolower_libc`). ICU and builtin don't set it. - For libc + single-byte: old took fast path, new takes fast path. ✓ - For ICU + single-byte: old took slow path, new takes slow path (no `char_tolower`). ✓ - For builtin + single-byte: builtin only supports UTF8, so `max_length == 1` is impossible. N/A. - For `ctype_is_c`: old behavior depended on provider/encoding; new always takes fast path. What does `ctype_is_c` mean? It's set for the C/POSIX libc locale and for the builtin "C" locale. In those cases the old code's behavior: - libc "C" with single-byte: old went fast path. New goes fast path. ✓ - libc "C" with multibyte (UTF8): old went slow path (called `lower()` then `MB_MatchText`). New goes fast path with `SB_IMatchText`. **This is a behavioral change.** With `ctype_is_c` and UTF8 encoding, the new code uses single-byte matching while the old used multibyte. For the C locale, ASCII-only case folding is correct byte-wise even in UTF8 (since multibyte sequences have only high-bit-set continuation bytes, and `SB_lower_char` with `ctype_is_c` only lowercases ASCII via `pg_ascii_tolower`). So this is likely intentional and correct — but it is a behavioral change worth noting. It may even be an improvement (skipping the `lower()` call). Actually, let me reconsider: `SB_IMatchText` operates byte-by-byte and uses `SB_lower_char`. With `ctype_is_c`, that returns `pg_ascii_tolower(c)`, which is safe on UTF8 continuation bytes (they're > 127, untouched). Pattern matching for LIKE with `_` wildcard might have problems in UTF8 though — `_` matches one character, but `SB_IMatchText` would match one *byte*. Looking at the existing code... yes, this was already a known consideration: the old code routed UTF8 to `MB_MatchText` precisely because `_` needs to match a character, not a byte. **So for `ctype_is_c` + UTF8, LIKE `_` will now match a single byte instead of a single character.** That's a real regression for queries like `'café' ILIKE 'caf_'` under the C locale in a UTF8 database. The old code would lowercase then call `MB_MatchText` (correct: `_` matches `é`). The new code calls `SB_IMatchText` which makes `_` match a single byte (incorrect: `é` is two bytes in UTF8, so `'caf_'` wouldn't match `'café'`). This looks like a **real bug**. ## Issue 5: `SB_lower_char` change Old: ```c return tolower_l(c, locale->info.lt); ``` New: ```c return char_tolower(c, locale); ``` `char_tolower` dispatches to `locale->ctype->char_tolower`. This is only set for libc. `SB_lower_char` is called from `SB_IMatchText`, which (in the old code) was only reached when `max_length == 1 && provider != ICU`. In the old code, all those cases were libc (since builtin requires UTF8). In the new code, `SB_IMatchText` can be reached for `ctype_is_c` regardless of provider — including builtin "C" (UTF8). Builtin doesn't set `char_tolower`, so `char_tolower(c, locale)` would crash with a NULL function pointer. But wait — let's look more carefully. In the `SB_lower_char` path, `locale->is_default` is checked first; if true, `pg_tolower` is used. For `ctype_is_c`, the code path enters this function only when `locale->ctype_is_c` is true. Looking at `SB_lower_char`: ```c if (locale->ctype_is_c) return pg_ascii_tolower(c); else if (locale->is_default) return pg_tolower(c); else return char_tolower(c, locale); ``` OK so when `ctype_is_c`, it returns early via `pg_ascii_tolower`. So no NULL deref. ✓ But the `_` wildcard byte-vs-character mismatch in UTF8 with `ctype_is_c` still stands as a real bug. Let me also check the `max_chr` field for builtin and ICU — they don't set it, which defaults to 0. In `pg_ctype_get_cache`: ```c if (pg_regex_locale->ctype->max_chr != 0 && pg_regex_locale->ctype->max_chr <= MAX_SIMPLE_CHR) { max_chr = pg_regex_locale->ctype->max_chr; pcc->cv.cclasscode = -1; } else max_chr = (pg_wchar) MAX_SIMPLE_CHR; ``` The original code for `LIBC_1BYTE` had: ```c #if MAX_SIMPLE_CHR >= UCHAR_MAX max_chr = (pg_wchar) UCHAR_MAX; pcc->cv.cclasscode = -1; #else max_chr = (pg_wchar) MAX_SIMPLE_CHR; #endif ``` This was conditional on `MAX_SIMPLE_CHR >= UCHAR_MAX`. New code drops the `#if` — it sets `max_chr = UCHAR_MAX` unconditionally when libc-sb method table is in use, and the runtime check `max_chr <= MAX_SIMPLE_CHR` handles the case where `MAX_SIMPLE_CHR < UCHAR_MAX`. So that's fine for production but the `cclasscode = -1` logic is preserved correctly. ✓ ## Summary of findings Here are the issues I'd flag, in order of severity: **1. Real bug — LIKE/ILIKE wildcard semantics change for C locale in UTF8 database.** In `Generic_Text_IC_like`, the new fast-path condition `locale->ctype_is_c || (char_tolower_enabled(locale) && pg_database_encoding_max_length() == 1)` sends C-locale UTF8 databases through `SB_IMatchText` (byte-by-byte) instead of `MB_MatchText` (character-by-character). This means `_` will match a single byte instead of a single character. The old condition guaranteed `SB_IMatchText` was used only when `max_length == 1`. Suggest: change the condition to also require `pg_database_encoding_max_length() == 1` for the `ctype_is_c` branch, or split it explicitly. **2. Lost bounds check on libc ctype calls.** The new `wc_is*_libc_sb` functions unconditionally do `isdigit_l((unsigned char) wc, ...)`, truncating `wc` silently. The original code had explicit `c <= UCHAR_MAX` guards that returned 0 for out-of-range values. The `max_chr = UCHAR_MAX` field protects the regex cache path, but direct calls to `pg_wc_isdigit` etc. for `wc > 255` will now return a result derived from a truncated byte rather than 0. Worth verifying whether any code path passes `wc > 255` to these. **3. Lost wchar_t-width fallback on 16-bit wchar_t platforms (Windows).** The old `LIBC_WIDE` branch had `if (sizeof(wchar_t) >= 4 || c <= 0xFFFF)` with `FALL THRU` to `LIBC_1BYTE` for codepoints above 0xFFFF on 16-bit-wchar_t platforms. The new `wc_is*_libc_mb` functions just call `iswdigit_l((wint_t) wc, ...)` unconditionally, and `toupper_libc_mb`/`tolower_libc_mb` return `wc` unchanged for `c > 0xFFFF` on 16-bit-wchar_t. The old fall-through path may have been doing something sketchy already, but it is a behavioral change on Windows. **4. Minor — comment block relocation note.** The big comment block moved from `regc_pg_locale.c` to `pg_locale_libc.c`. The paragraph "We store the active collation setting in static variables. In principle it could be passed down to here via the regex library's 'struct vars' data structure…" is about the regex library specifically and now lives in the libc file where it's somewhat out of place. Not a bug, just slightly misleading documentation. **5. Style — the new `pg_wc_toupper`/`pg_wc_tolower` `ctype_is_c` blocks** drop the explicit `case`-style brace indentation but are otherwise faithful; the `return c;` for `c > 127` is preserved. Issue #1 looks like the most serious — it'd be worth writing a regression test like `SELECT 'café' ILIKE 'caf_' COLLATE "C"` in a UTF8 database to confirm.