Make uselocale protection more consistent
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t48073psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 28, 2026 at 12:30 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t48073_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t48073_1 && git checkout t48073_1Patchset v1 (message #1) is on t48073_1
This is a patch which implements an issue discussed in bug #17946[0]/messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.
[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
--
Tristan Partin
Neon (https://neon.tech)
On 27.06.23 17:02, Tristan Partin wrote:
This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.
I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.
On Mon, Jul 3, 2023 at 6:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 27.06.23 17:02, Tristan Partin wrote:
This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.
I propose[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().
[1]: /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
On Mon Jul 3, 2023 at 1:24 AM CDT, Thomas Munro wrote:
On Mon, Jul 3, 2023 at 6:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 27.06.23 17:02, Tristan Partin wrote:
This is a patch which implements an issue discussed in bug #17946[0]. It
doesn't fix the overarching issue of the bug, but merely a consistency
issue which was found while analyzing code by Heikki. I had originally
submitted the patch within that thread, but for visibility and the
purposes of the commitfest, I have re-sent it in its own thread.[0]: /messages/by-id/49dfcad8-90fa-8577-008f-d142e61af46b@iki.fi
I notice that HAVE_USELOCALE was introduced much later than
HAVE_LOCALE_T, and at the time the code was already using uselocale(),
so perhaps the introduction of HAVE_USELOCALE was unnecessary and should
be reverted.I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1] /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.
--
Tristan Partin
Neon (https://neon.tech)
On 03.07.23 08:24, Thomas Munro wrote:
I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1] /messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
Ok, it appears your patch is imminent, so let's wait for that and see if
this patch here is still required afterwards.
On 03.07.23 15:21, Tristan Partin wrote:
I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.
The current code is structured
#ifdef HAVE_LOCALE_T
#ifdef HAVE_WCSTOMBS_L
wcstombs_l(...);
#else
uselocale(...);
#endif
#else
elog(ERROR);
#endif
If you just replace HAVE_LOCALE_T with HAVE_USELOCALE, then this would
penalize a platform that has wcstombs_l(), but not uselocale(). I think
the correct structure would be
#if defined(HAVE_WCSTOMBS_L)
wcstombs_l(...);
#elif defined(HAVE_USELOCALE)
uselocale(...);
#else
elog(ERROR);
#endif
On Mon Jul 3, 2023 at 9:21 AM CDT, Peter Eisentraut wrote:
On 03.07.23 15:21, Tristan Partin wrote:
I think it would be better to keep HAVE_LOCALE_T as encompassing any of
the various locale_t-using functions, rather than using HAVE_USELOCALE
as a proxy for them. Otherwise you create weird situations like having
#ifdef HAVE_WCSTOMBS_L inside #ifdef HAVE_USELOCALE, which doesn't make
sense, I think.I propose[1] that we get rid of HAVE_LOCALE_T completely and make
"libc" provider support unconditional. It's standardised, and every
target system has it, even Windows. But Windows doesn't have
uselocale().[1]/messages/by-id/CA+hUKGL7CmmzeRhoirzjECmOdABVFTn8fo6gEOaFRF1Oxey6Hw@mail.gmail.com
I think keeping HAVE_USELOCALE is important for the Windows case as
mentioned. I need it for my localization work where I am ripping out
setlocale() on non-Windows.The current code is structured
#ifdef HAVE_LOCALE_T
#ifdef HAVE_WCSTOMBS_L
wcstombs_l(...);
#else
uselocale(...);
#endif
#else
elog(ERROR);
#endifIf you just replace HAVE_LOCALE_T with HAVE_USELOCALE, then this would
penalize a platform that has wcstombs_l(), but not uselocale(). I think
the correct structure would be#if defined(HAVE_WCSTOMBS_L)
wcstombs_l(...);
#elif defined(HAVE_USELOCALE)
uselocale(...);
#else
elog(ERROR);
#endif
That makes sense to me. I gave it some more thought. Maybe it makes more
sense to just completely drop HAVE_USELOCALE as mentioned, and protect
calls to it with #ifdef WIN32 or whatever the macro is. HAVE_USELOCALE
might be more descriptive, but I don't really care that much either way.
--
Tristan Partin
Neon (https://neon.tech)