Re: Crash issue in PG18.5 regression

Hi,

On 2026-08-15 14:53:39 -0700, Jeff Davis wrote:
> On Thu, 2026-08-13 at 22:17 -0700, Jeff Davis wrote:
> > On Tue, 2026-08-11 at 14:31 -0400, Andres Freund wrote:
> > > Hm. If I infer the pg_strlower() API correctly - it's utterly
> > > underdocumented
> >
> > Agreed. Patch attached.
> >
> > > 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.
> >
> > Patch attached for that, too.
> >
> > I also attached patches to make all the functions work with
> > collate_is_c, and fixed up the -1 API in 18.
>
> Now with a C test module (made with AI assistance).
>
> I plan to start committing these fairly soon. I'm not sure whether to
> backport the C test module, but I included the patches to do so.

Thanks for working on these.

> From f669e9fafb9bf435c946dcc2c2dd98efb4aa46cb Mon Sep 17 00:00:00 2001
> From: Jeff Davis <jeff(at)j-davis(dot)com>
> Date: Thu, 13 Aug 2026 21:12:34 -0700
> Subject: [PATCH vPG18 2/5] pg_locale.c, unicode_case.c: use size_t for
> iteration.
>
> No actual problem, just cleanup. Only relevant to 18 and 19.

FWIW, I think this is actually a bug, it just turns out that we don't know of
any callers that hit it.

> From a8e72bb34c979ac43ee8480fd10ed4e73db47b10 Mon Sep 17 00:00:00 2001
> From: Jeff Davis <jeff(at)j-davis(dot)com>
> Date: Wed, 12 Aug 2026 07:32:19 -0700
> Subject: [PATCH vPG18 3/5] Add missing comments in pg_locale.c.
>
> Suggested-by: Andres Freund <andres(at)anarazel(dot)de>
> Discussion: https://postgr.es/m/v3nniwcrxejmcfvz56xbd22hphprqleuornd6hqkmw2bl7kgmz@cnytz2ee5ltk
> Backpatch-through: 18

Nice, that's much better than before.

> +/*
> + * pg_strtitle()
> + *
> + * Convert src to titlecase, and return the result length (not including
> + * terminating NUL).

Might not hurt to actually say what titlecase and folded strings are.

> From adeef54179a11f5b7817054bc53f8b7b584d2b6a Mon Sep 17 00:00:00 2001
> From: Jeff Davis <jeff(at)j-davis(dot)com>
> Date: Sat, 15 Aug 2026 12:26:09 -0700
> Subject: [PATCH vPG18 5/5] Add C test module for pg_locale.h APIs.
>
> Test the API independently to account for fallback paths that aren't
> adequately tested from SQL.
>
> The backport to 18 also tests the previously-supported behavior where
> a size of -1 meant that the string was NUL-terminated. That behavior
> was later removed in 19.
>
> Discussion: https://postgr.es/m/v36ssaygf7grb3qzfsjhtdzi7kqd45ds56nyuf7gi5qjml4qbb@ezmfqzmhlrs2
> Backpatch-through: 18
> ---
> src/test/modules/Makefile | 1 +
> src/test/modules/meson.build | 1 +
> src/test/modules/test_pg_locale/.gitignore | 4 +
> src/test/modules/test_pg_locale/Makefile | 23 +++
> src/test/modules/test_pg_locale/README | 2 +
> .../expected/test_pg_locale.out | 38 ++++
> src/test/modules/test_pg_locale/meson.build | 33 ++++
> .../test_pg_locale/sql/test_pg_locale.sql | 23 +++
> .../test_pg_locale/test_pg_locale--1.0.sql | 8 +
> .../modules/test_pg_locale/test_pg_locale.c | 169 ++++++++++++++++++
> .../test_pg_locale/test_pg_locale.control | 4 +
> 11 files changed, 306 insertions(+)
> create mode 100644 src/test/modules/test_pg_locale/.gitignore
> create mode 100644 src/test/modules/test_pg_locale/Makefile
> create mode 100644 src/test/modules/test_pg_locale/README
> create mode 100644 src/test/modules/test_pg_locale/expected/test_pg_locale.out
> create mode 100644 src/test/modules/test_pg_locale/meson.build
> create mode 100644 src/test/modules/test_pg_locale/sql/test_pg_locale.sql
> create mode 100644 src/test/modules/test_pg_locale/test_pg_locale--1.0.sql
> create mode 100644 src/test/modules/test_pg_locale/test_pg_locale.c
> create mode 100644 src/test/modules/test_pg_locale/test_pg_locale.control

I very much like that this is tested now, but this really need its own
initdb'd cluster? Every full testrun writes ginormous amounts of data (~73GB
for one master run on macos!), due to the number of clusters we create, and
the amount is growing from release to release at an alarming clip.

Sometimes that's unavoidable, because you need a server configured in a
specific way, the tests take a good while and should therefore run
concurrently, or such. But that shouldn't be the case her. Can't you stuff
this into regress.c or such?

Other than that complaint, I'd probably backpatch this. Seems unlikely to be
flappy or such?

Greetings,

Andres Freund