Re: TM format can mix encodings in to_char()

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Juan José Santamaría Flecha <juanjo(dot)santamaria(at)gmail(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: TM format can mix encodings in to_char()
Date: 2019-04-20 18:46:12
Message-ID: 8523.1555785972@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> This is a little bit off, now that I look at it, because it's
> failing to account for the possibility of getting -1 from
> pg_get_encoding_from_locale. It should probably do what
> pg_bind_textdomain_codeset does:
> if (encoding < 0)
> encoding = PG_SQL_ASCII;

Actually, the more I looked at the existing code, the less happy I got
with its error handling. cache_locale_time() is an absolute failure
in that respect, because it makes no attempt at all to avoid throwing
errors while LC_TIME or LC_CTYPE is set to a transient value. We
could maybe tolerate LC_TIME being weird, but continuing with a value
of LC_CTYPE that doesn't match the database setting would almost
certainly be disastrous.

PGLC_localeconv had at least thought about the issue, but it ends up
wimping out:

/*
* Report it if we failed to restore anything. Perhaps this should be
* FATAL, rather than continuing with bad locale settings?
*/
if (trouble)
elog(WARNING, "failed to restore old locale");

And it's also oddly willing to keep going even if it couldn't get the
original setlocale settings to begin with.

I think that this code was written with only LC_MONETARY and LC_NUMERIC
in mind, for which there's at least some small argument that continuing
with unwanted values wouldn't be fatal (though IIRC, LC_NUMERIC would
still change the behavior of float8out). Since we added code to also
change LC_CTYPE on Windows, I think that continuing on after a restore
failure would be disastrous. And we've not heard any field reports
of this warning anyway, so there's no reason to think we have to support
the case in practice.

Hence, the attached revised patch changes the code to do elog(FATAL)
if it can't restore any locale settings to their previous values,
and it fixes cache_locale_time to not do anything risky while it's
got transient locale settings in place.

I propose to apply and back-patch this; the code's basically the
same in all supported branches.

regards, tom lane

Attachment Content-Type Size
fix-encoding-and-error-recovery-in-cache-locale-time.patch text/x-diff 12.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2019-04-20 20:11:11 Re: block-level incremental backup
Previous Message Alexander Korotkov 2019-04-20 18:01:17 Re: jsonpath