Remaining dependency on setlocale()
After some previous work here:
/messages/by-id/89475ee5487d795124f4e25118ea8f1853edb8cb.camel@j-davis.com
we are less dependent on setlocale(), but it's still not completely
gone.
setlocale() counts as thread-unsafe, so it would be nice to eliminate
it completely.
The obvious answer is uselocale(), which sets the locale only for the
calling thread, and takes precedence over whatever is set with
setlocale().
But there are a couple problems:
1. I don't think it's supported on Windows.
2. I don't see a good way to canonicalize a locale name, like in
check_locale(), which uses the result of setlocale().
Thoughts?
Regards,
Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes:
But there are a couple problems:
1. I don't think it's supported on Windows.
Can't help with that, but surely Windows has some thread-safe way.
2. I don't see a good way to canonicalize a locale name, like in
check_locale(), which uses the result of setlocale().
What I can tell you about that is that check_locale's expectation
that setlocale does any useful canonicalization is mostly wishful
thinking [1]/messages/by-id/14856.1348497531@sss.pgh.pa.us. On a lot of platforms you just get the input string
back again. If that's the only thing keeping us on setlocale,
I think we could drop it. (Perhaps we should do some canonicalization
of our own instead?)
regards, tom lane
On Wed, Aug 7, 2024 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jeff Davis <pgsql@j-davis.com> writes:
But there are a couple problems:
1. I don't think it's supported on Windows.
Can't help with that, but surely Windows has some thread-safe way.
It does. It's not exactly the same, instead there is a thing you can
call that puts setlocale() itself into a thread-local mode, but last
time I checked that mode was missing on MinGW so that's a bit of an
obstacle.
How far can we get by using more _l() functions? For example, [1]/messages/by-id/CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com
shows a use of strftime() that I think can be converted to
strftime_l() so that it doesn't depend on setlocale(). Since POSIX
doesn't specify every obvious _l function, we might need to provide
any missing wrappers that save/restore thread-locally with
uselocale(). Windows doesn't have uselocale(), but it generally
doesn't need such wrappers because it does have most of the obvious
_l() functions.
2. I don't see a good way to canonicalize a locale name, like in
check_locale(), which uses the result of setlocale().What I can tell you about that is that check_locale's expectation
that setlocale does any useful canonicalization is mostly wishful
thinking [1]. On a lot of platforms you just get the input string
back again. If that's the only thing keeping us on setlocale,
I think we could drop it. (Perhaps we should do some canonicalization
of our own instead?)
+1
I know it does something on Windows (we know the EDB installer gives
it strings like "Language,Country" and it converts them to
"Language_Country.Encoding", see various threads about it all going
wrong), but I'm not sure it does anything we actually want to
encourage. I'm hoping we can gradually screw it down so that we only
have sane BCP 47 in the system on that OS, and I don't see why we
wouldn't just use them verbatim.
[1]: /messages/by-id/CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com
On 8/7/24 03:07, Thomas Munro wrote:
How far can we get by using more _l() functions? For example, [1]
shows a use of strftime() that I think can be converted to
strftime_l() so that it doesn't depend on setlocale(). Since POSIX
doesn't specify every obvious _l function, we might need to provide
any missing wrappers that save/restore thread-locally with
uselocale(). Windows doesn't have uselocale(), but it generally
doesn't need such wrappers because it does have most of the obvious
_l() functions.
Most of the strtoX functions have an _l variant, but one to watch is
atoi, which is defined with a hardcoded call to strtol, at least with glibc:
8<----------
/* Convert a string to an int. */
int
atoi (const char *nptr)
{
return (int) strtol (nptr, (char **) NULL, 10);
}
8<----------
I guess in many/most places we use atoi we don't care, but maybe it
matters for some?
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Aug 7, 2024 at 9:42 AM Joe Conway <mail@joeconway.com> wrote:
I guess in many/most places we use atoi we don't care, but maybe it
matters for some?
I think we should move in the direction of replacing atoi() calls with
strtol() and actually checking for errors. In many places where use
atoi(), it's unlikely that the string would be anything but an
integer, so error checks are arguably unnecessary. A backup label file
isn't likely to say "START TIMELINE: potaytoes". On the other hand, if
it did say that, I'd prefer to get an error about potaytoes than have
it be treated as if it said "START TIMELINE: 0". And I've definitely
found missing error-checks over the years. For example, on pg14,
"pg_basebackup -Ft -Zmaximum -Dx" works as if you specified "-Z0"
because atoi("maximum") == 0. If we make a practice of checking
integer conversions for errors everywhere, we might avoid some such
silliness.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, 2024-08-07 at 19:07 +1200, Thomas Munro wrote:
How far can we get by using more _l() functions?
There are a ton of calls to, for example, isspace(), used mostly for
parsing.
I wouldn't expect a lot of differences in behavior from locale to
locale, like might be the case with iswspace(), but behavior can be
different at least in theory.
So I guess we're stuck with setlocale()/uselocale() for a while, unless
we're able to move most of those call sites over to an ascii-only
variant.
Regards,
Jeff Davis
On 8/7/24 13:16, Jeff Davis wrote:
On Wed, 2024-08-07 at 19:07 +1200, Thomas Munro wrote:
How far can we get by using more _l() functions?
There are a ton of calls to, for example, isspace(), used mostly for
parsing.I wouldn't expect a lot of differences in behavior from locale to
locale, like might be the case with iswspace(), but behavior can be
different at least in theory.So I guess we're stuck with setlocale()/uselocale() for a while, unless
we're able to move most of those call sites over to an ascii-only
variant.
FWIW I see all of these in glibc:
isalnum_l, isalpha_l, isascii_l, isblank_l, iscntrl_l, isdigit_l,
isgraph_l, islower_l, isprint_l, ispunct_l, isspace_l, isupper_l,
isxdigit_l
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Aug 7, 2024 at 1:29 PM Joe Conway <mail@joeconway.com> wrote:
FWIW I see all of these in glibc:
isalnum_l, isalpha_l, isascii_l, isblank_l, iscntrl_l, isdigit_l,
isgraph_l, islower_l, isprint_l, ispunct_l, isspace_l, isupper_l,
isxdigit_l
On my MacBook (Ventura, 13.6.7), I see all of these except for isascii_l.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Aug 8, 2024 at 5:16 AM Jeff Davis <pgsql@j-davis.com> wrote:
There are a ton of calls to, for example, isspace(), used mostly for
parsing.I wouldn't expect a lot of differences in behavior from locale to
locale, like might be the case with iswspace(), but behavior can be
different at least in theory.So I guess we're stuck with setlocale()/uselocale() for a while, unless
we're able to move most of those call sites over to an ascii-only
variant.
We do know of a few isspace() calls that are already questionable[1]/messages/by-id/CA+HWA9awUW0+RV_gO9r1ABZwGoZxPztcJxPy8vMFSTbTfi4jig@mail.gmail.com
(should be scanner_isspace(), or something like that). It's not only
weird that SELECT ROW('libertà!') is displayed with or without double
quote depending (in theory) on your locale, it's also undefined
behaviour because we feed individual bytes of a multi-byte sequence to
isspace(), so OSes disagree, and in practice we know that macOS and
Windows think that the byte 0xa inside 'à' is a space while glibc and
FreeBSD don't. Looking at the languages with many sequences
containing 0xa0, I guess you'd probably need to be processing CJK text
and cross-platform for the difference to become obvious (that was the
case for the problem report I analysed):
for i in range(1, 0xffff):
if (i < 0xd800 or i > 0xdfff) and 0xa0 in chr(i).encode('UTF-8'):
print("%04x: %s" % (i, chr(i)))
[1]: /messages/by-id/CA+HWA9awUW0+RV_gO9r1ABZwGoZxPztcJxPy8vMFSTbTfi4jig@mail.gmail.com
On Thu, Aug 8, 2024 at 6:18 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Aug 7, 2024 at 1:29 PM Joe Conway <mail@joeconway.com> wrote:
FWIW I see all of these in glibc:
isalnum_l, isalpha_l, isascii_l, isblank_l, iscntrl_l, isdigit_l,
isgraph_l, islower_l, isprint_l, ispunct_l, isspace_l, isupper_l,
isxdigit_lOn my MacBook (Ventura, 13.6.7), I see all of these except for isascii_l.
Those (except isascii_l) are from POSIX 2008[1]https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/isspace.html. They were absorbed
from "Extended API Set Part 4"[2]https://pubs.opengroup.org/onlinepubs/9699939499/toc.pdf, along with locale_t (that's why
there is a header <xlocale.h> on a couple of systems even though after
absorption they are supposed to be in <locale.h>). We already
decided that all computers have that stuff (commit 8d9a9f03), but the
reality is a little messier than that... NetBSD hasn't implemented
uselocale() yet[3]/messages/by-id/CWZBBRR6YA8D.8EHMDRGLCKCD@neon.tech, though it has a good set of _l functions. As
discussed in [3]/messages/by-id/CWZBBRR6YA8D.8EHMDRGLCKCD@neon.tech, ECPG code is therefore currently broken in
multithreaded clients because it's falling back to a setlocale() path,
and I think Windows+MinGW must be too (it lacks
HAVE__CONFIGTHREADLOCALE), but those both have a good set of _l
functions. In that thread I tried to figure out how to use _l
functions to fix that problem, but ...
The issue there is that we have our own snprintf.c, that implicitly
requires LC_NUMERIC to be "C" (it is documented as always printing
floats a certain way ignoring locale and that's what the callers there
want in frontend and backend code, but in reality it punts to system
snprintf for floats, assuming that LC_NUMERIC is "C", which we
configure early in backend startup, but frontend code has to do it for
itself!). So we could use snprintf_l or strtod_l instead, but POSIX
hasn't got those yet. Or we could use own own Ryu code (fairly
specific), but integrating Ryu into our snprintf.c (and correctly
implementing all the %... stuff?) sounds like quite a hard,
devil-in-the-details kind of an undertaking to me. Or maybe it's
easy, I dunno. As for the _l functions, you could probably get away
with "every computer has either uselocale() or snprintf_() (or
strtod_()?)" and have two code paths in our snprintf.c. But then we'd
also need a place to track a locale_t for a long-lived newlocale("C"),
which was too messy in my latest attempt...
[1]: https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/isspace.html
[2]: https://pubs.opengroup.org/onlinepubs/9699939499/toc.pdf
[3]: /messages/by-id/CWZBBRR6YA8D.8EHMDRGLCKCD@neon.tech
On Wed, 2024-08-07 at 13:28 -0400, Joe Conway wrote:
FWIW I see all of these in glibc:
isalnum_l, isalpha_l, isascii_l, isblank_l, iscntrl_l, isdigit_l,
isgraph_l, islower_l, isprint_l, ispunct_l, isspace_l, isupper_l,
isxdigit_l
My point was just that there are a lot of those call sites (especially
for isspace()) in various parsers. It feels like a lot of code churn to
change all of them, when a lot of them seem to be intended for ascii
anyway.
And where do we get the locale_t structure from? We can create our own
at database connection time, and supply it to each of those call sites,
but I'm not sure that's a huge advantage over just using uselocale().
Regards,
Jeff Davis
On 8/8/24 12:45 AM, Jeff Davis wrote:
My point was just that there are a lot of those call sites (especially
for isspace()) in various parsers. It feels like a lot of code churn to
change all of them, when a lot of them seem to be intended for ascii
anyway.And where do we get the locale_t structure from? We can create our own
at database connection time, and supply it to each of those call sites,
but I'm not sure that's a huge advantage over just using uselocale().
I am leaning towards that we should write our own pure ascii functions
for this. Since we do not support any non-ascii compatible encodings
anyway I do not see the point in having locale support in most of these
call-sites.
Andewas
On Fri, 2024-08-09 at 13:41 +0200, Andreas Karlsson wrote:
I am leaning towards that we should write our own pure ascii
functions
for this.
That makes sense for a lot of call sites, but it could cause breakage
if we aren't careful.
Since we do not support any non-ascii compatible encodings
anyway I do not see the point in having locale support in most of
these
call-sites.
An ascii-compatible encoding just means that the code points in the
ascii range are represented as ascii. I'm not clear on whether code
points in the ascii range can return different results for things like
isspace(), but it sounds plausible -- toupper() can return different
results for 'i' in tr_TR.
Also, what about the values outside 128-255, which are still valid
input to isspace()?
Regards,
Jeff Davis
On Tue Aug 6, 2024 at 5:00 PM CDT, Jeff Davis wrote:
After some previous work here:
/messages/by-id/89475ee5487d795124f4e25118ea8f1853edb8cb.camel@j-davis.com
we are less dependent on setlocale(), but it's still not completely
gone.setlocale() counts as thread-unsafe, so it would be nice to eliminate
it completely.The obvious answer is uselocale(), which sets the locale only for the
calling thread, and takes precedence over whatever is set with
setlocale().But there are a couple problems:
1. I don't think it's supported on Windows.
2. I don't see a good way to canonicalize a locale name, like in
check_locale(), which uses the result of setlocale().Thoughts?
Hey Jeff,
See this thread[0]/messages/by-id/CWMW5OZBWJ10.1YFLQWSUE5RE9@neon.tech for some work that I had previously done. Feel free
to take it over, or we could collaborate.
[0]: /messages/by-id/CWMW5OZBWJ10.1YFLQWSUE5RE9@neon.tech
--
Tristan Partin
Neon (https://neon.tech)
Hi,
On Fri, 2024-08-09 at 15:16 -0500, Tristan Partin wrote:
Hey Jeff,
See this thread[0] for some work that I had previously done. Feel
free
to take it over, or we could collaborate.
Sounds good, sorry I missed that.
Can you please rebase and we can discuss in that thread?
Regards,
Jeff Davis
On Wed, Aug 7, 2024 at 7:07 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Wed, Aug 7, 2024 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jeff Davis <pgsql@j-davis.com> writes:
But there are a couple problems:
1. I don't think it's supported on Windows.
Can't help with that, but surely Windows has some thread-safe way.
It does. It's not exactly the same, instead there is a thing you can
call that puts setlocale() itself into a thread-local mode, but last
time I checked that mode was missing on MinGW so that's a bit of an
obstacle.
Actually the MinGW situation might be better than that these days. I
know of three environments where we currently have to keep code
working on MinGW: build farm animal fairywren (msys2 compiler
toochain), CI's optional "Windows - Server 2019, MinGW64 - Meson"
task, and CI's "CompilerWarnings" task, in the "mingw_cross_warning"
step (which actually runs on Linux, and uses configure rather than
meson). All three environments show that they have
_configthreadlocale. So could we could simply require it on Windows?
Then it might be possible to write a replacement implementation of
uselocale() that does a two-step dance with _configthreadlocale() and
setlocale(), restoring both afterwards if they changed. That's what
ECPG open-codes already.
The NetBSD situation is more vexing. I was trying to find out if
someone is working on it and unfortunately it looks like there is a
principled stand against adding it:
https://mail-index.netbsd.org/tech-userlevel/2015/12/28/msg009546.html
https://mail-index.netbsd.org/netbsd-users/2017/02/14/msg019352.html
They're right that we really just want to use "C" in some places, and
their LC_C_LOCALE is a very useful system-provided value to be able to
pass into _l functions. It's a shame it's non-standard, because
without it you have to allocate a locale_t for "C" and keep it
somewhere to feed to _l functions...
I've posted a new attempt at ripping those ECPG setlocales() out on
the other thread that had the earlier version and discussion:
/messages/by-id/CA+hUKG+Yv+ps=nS2T8SS1UDU=iySHSr4sGHYiYGkPTpZx6Ooww@mail.gmail.com
On Thu, Aug 8, 2024 at 5:16 AM Jeff Davis <pgsql@j-davis.com> wrote:
On Wed, 2024-08-07 at 19:07 +1200, Thomas Munro wrote:
How far can we get by using more _l() functions?
There are a ton of calls to, for example, isspace(), used mostly for
parsing.I wouldn't expect a lot of differences in behavior from locale to
locale, like might be the case with iswspace(), but behavior can be
different at least in theory.So I guess we're stuck with setlocale()/uselocale() for a while, unless
we're able to move most of those call sites over to an ascii-only
variant.
Here are two more cases that I don't think I've seen discussed.
1. The nl_langinfo() call in pg_get_encoding_from_locale(), can
probably be changed to nl_langinfo_l() (it is everywhere we currently
care about except Windows, which has a different already-thread-safe
alternative; AIX seems to lack the _l version, but someone writing a
patch to re-add support for that OS could supply the configure goo for
a uselocale() safe/restore implementation). One problem is that it
has callers that pass it NULL meaning the backend default, but we'd
perhaps use LC_C_GLOBAL for now and have to think about where we get
the database default locale_t in the future.
2. localeconv() is *doubly* non-thread-safe: it depends on the
current locale, and it also returns an object whose storage might be
clobbered by any other call to localeconv(), setlocale, or even,
according to POSIX, uselocale() (!!!). I think that effectively
closes off that escape hatch. On some OSes (macOS, BSDs) you find
localeconv_l() and then I think they give you a more workable
lifetime: as long as the locale_t lives, which makes perfect sense. I
am surprised that no one has invented localeconv_r() where you supply
the output storage, and you could wrap that in uselocale()
save/restore to deal with the other problem, or localeconv_r_l() or
something. I can't understand why this is so bad. The glibc
documentation calls it "a masterpiece of poor design". Ahh, so it
seems like we need to delete our use of localeconf() completely,
because we should be able to get all the information we need from
nl_langinfo_l() instead:
https://www.gnu.org/software/libc/manual/html_node/Locale-Information.html
On Mon, Aug 12, 2024 at 3:24 PM Thomas Munro <thomas.munro@gmail.com> wrote:
1. The nl_langinfo() call in pg_get_encoding_from_locale(), can
probably be changed to nl_langinfo_l() (it is everywhere we currently
care about except Windows, which has a different already-thread-safe
alternative ...
... though if we wanted to replace all use of localeconv and struct
lconv with nl_langinfo_l() calls, it's not totally obvious how to do
that on Windows. Its closest thing is GetLocaleInfoEx(), but that has
complications: it takes wchar_t locale names, which we don't even have
and can't access when we only have a locale_t, and it must look them
up in some data structure every time, and it copies data out to the
caller as wchar_t so now you have two conversion problems and a
storage problem. If I understand correctly, the whole point of
nl_langinfo_l(item, loc) is that it is supposed to be fast, it's
really just an array lookup, and item is just an index, and the result
is supposed to be stable as long as loc hasn't been freed (and the
thread hasn't exited). So you can use it without putting your own
caching in front of it. One idea I came up with which I haven't tried
and it might turn out to be terrible, is that we could change our
definition of locale_t on Windows. Currently it's a typedef to
Windows' _locale_t, and we use it with a bunch of _XXX functions that
we access by macro to remove the underscore. Instead, we could make
locale_t a pointer to a struct of our own design in WIN32 builds,
holding the native _locale_t and also an array full of all the values
that nl_langinfo_l() can return. We'd provide the standard enums,
indexes into that array, in a fake POSIX-oid header <langinfo.h>.
Then nl_langinfo_l(item, loc) could be implemented as
loc->private_langinfo[item], and strcoll_l(.., loc) could be a static
inline function that does _strcol_l(...,
loc->private_windows_locale_t). These structs would be allocated and
freed with standard-looking newlocale() and freelocale(), so we could
finally stop using #ifdef WIN32-wrapped _create_locale() directly.
Then everything would look more POSIX-y, nl_langinfo_l() could be used
directly wherever we need fast access to that info, and we could, I
think, banish the awkward localeconv, right? I don't know if this all
makes total sense and haven't tried it, just spitballing here...
On Mon, Aug 12, 2024 at 4:53 PM Thomas Munro <thomas.munro@gmail.com> wrote:
... though if we wanted to replace all use of localeconv and struct
lconv with nl_langinfo_l() calls,
Gah. I realised while trying the above that you can't really replace
localeconv() with nl_langinfo_l() as the GNU documentation recommends,
because some of the lconv fields we're using are missing from
langinfo.h in POSIX (only GNU added them all, that was a good idea).
:-(
Next idea:
Windows: its localeconv() returns pointer to thread-local storage,
good, but it still needs setlocale(). So the options are: make our
own lconv-populator function for Windows, using GetLocaleInfoEx(), or
do that _configthreadlocale() dance (possibly excluding some MinGW
configurations from working)
Systems that have localeconv_l(): use that
POSIX: use uselocale() and also put a big global lock around
localeconv() call + accessing result (optionally skipping that on an
OS-by-OS basis after confirming that its implementation doesn't really
need it)
The reason the uselocale() + localeconv() seems to require a Big Lock
(by default at least) is that the uselocale() deals only with the
"current locale" aspect, not the output buffer aspect. Clearly the
standard allows for it to be thread-local storage (that's why since
2008 it says that after thread-exit you can't access the result, and I
guess that's how it works on real systems (?)), but it also seems to
allow for a single static buffer (that's why it says that it's not
re-entrant, and any call to localeconv() might clobber it). That
might be OK in practice because we tend to cache that stuff, eg when
assigning GUC lc_monetary (that cache would presumably become
thread-local in the first phase of the multithreading plan), so the
locking shouldn't really hurt.
The reason we'd have to have three ways, and not just two, is again
that NetBSD declined to implement uselocale().
I'll try this in a bit unless someone else has better ideas or plans
for this part... sorry for the drip-feeding.
