Re: On non-Windows, hard depend on uselocale(3)

On Thu, Aug 29, 2024 at 6:50 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> I took a look at this. It was quite a complicated discussion that led
> to this, but I agree with the solution that was arrived at.

Sorry I missed/forgot this prior feedback when posting earlier today.
Responses to the points you raised here:

> Also, you're removing the configure test for _configthreadlocale().
> Presumably because you're removing all the uses. But wouldn't we need
> that back later in the backend maybe? Or is that test even relevant
> anymore, that is, are there Windows versions that don't have it?

Yeah, this removes the uses. As it happens, in the thread about
localeconv() I am also proposing to add a new user of it[1], but even
then I don't think we'd need the configure test, because all relevant
systems have it. It's in ucrt, the modern C runtime that replaced the
old msvcrt, as used by Visual Studio 2015+ and Windows 10+, and also
by MinGW in recent years. I think it was also in some versions of
msvcrt, but MinGW had issues with that apparently.

It's pretty confusing, so I went looking for proof and clues about the
history. Here's a thread about 2019 animal jacana lacking the
function:

https://www.postgresql.org/message-id/flat/31420.1547783697%40sss.pgh.pa.us

Once jacana was upgraded, it started finding the function, but next
failed with -1 at runtime, cf 2cf91ccb. Grovelling through
MinGW-w64's github, it looks like they added a dummy function that
fails with -1 in 2016:

https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/misc/_configthreadlocale.c#L11

They only seem to interpose the dummy function when building against msvcrt.

https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-crt/Makefile.am#L298

The default and recommended runtime library as of a few years ago is ucrt:

https://www.msys2.org/docs/environments/#msvcrt-vs-ucrt

The three MinGW environments we test today are using ucrt, and
configure detects the symbol on all. Namely: fairwren
(msys2/mingw64), the CI mingw64 task and the mingw cross-build that
runs on Linux in the CI CompilerWarnings task. As far as I know these
are the reasons for, and mechanism by which, we keep MinGW support
working. We have no policy requiring arbitrary old MinGW systems
work, and we wouldn't know anyway.

I was paranoid that it might still be a dummy function, so I tried
running a simple program on our CI with the MinGW compiler to confirm
it's reaching the real library:

int
main()
{
printf("hello world\n");
printf("got %d\n", _configthreadlocale(_ENABLE_PER_THREAD_LOCALE));
printf("got %d\n", _configthreadlocale(_DISABLE_PER_THREAD_LOCALE));
}

[06:16:39.580] c:\cirrus>call C:\msys64\usr\bin\bash.exe -l -c "gcc
doit.c -o c:/doit"
...
[06:16:41.474] c:\cirrus>call c:\doit.exe
[06:16:41.482] hello world
[06:16:41.482] got 2
[06:16:41.482] got 1

It's not clear to me whether or when we might have made any changes
already that prevented PostgreSQL from working with msvcrt, of course,
but I'm pretty sure if someone proposed that we *should* support it,
we'd reject the proposal. That'd effectively be a different, obsolete
one corresponding to Windows versions we've dropped, which wouldn't
make any sense.

Other mentions of msvcrt in win32env.c and pg_locale.c may also be
effectively obsolete now too.

> For Windows, we already have things like
>
> #define strcoll_l _strcoll_l
>
> in src/include/port/win32_port.h, so it would seem more sensible to add
> strtod_l to that list, instead of in port.h.

OK, I moved that line into that list. I left the implementation for
Unixen (at a guess: probably just Solaris?) there.

> The error handling with pg_ensure_c_locale(), that's the sort of thing
> I'm afraid will be hard to test or even know how it will behave. And it
> creates this weird coupling between pgtypeslib and ecpglib that you
> mentioned earlier. And if there are other users of PG_C_LOCALE in the
> future, there will be similar questions about the proper initialization
> and error handling sequence.

Ack. The coupling does become at least less weird (currently it must
be capable of giving the wrong answers reliably if called directly I
think, no?) and weaker, but I acknowledge that it's still non-ideal
that out-of-memory would be handled nicely only if you used ecpg
first, and subtle misbehaviour would ensure otherwise, and future
users face the same sort of problem unless they design in a reasonable
initialisation place that could check pg_ensure_c_locale() nicely.
Classic retro-fitting problem, though.

> I would consider instead making a local static variable in each function
> that needs this. For example, numericvar_to_double() might do
>
> {
> static locale_t c_locale;
>
> if (!c_locale)
> {
> c_locale = pg_get_c_locale();
> if (!c_locale)
> return -1; /* local error reporting convention */
> }
>
> ...
> }
>
> This is a bit more code in total, but then you only initialize what you
> need and you can handle errors locally.

Hmm. Seems like quite a lot of duplication, and also assumes that we
can actually unwind and handle the error in a reasonable way at all
callsites. Let me think about that some more...

[1] https://www.postgresql.org/message-id/flat/CA+hUKGJqVe0+Pv9dvC9dSums_PXxGo9SWcxYAMBguWJUGbWz-A(at)mail(dot)gmail(dot)com