check_locale() and the empty string

On Sun, 2012-03-11 at 11:20 -0700, Jeff Davis wrote:

The problem seems to be in check_locale(), which just checks for a
non-NULL return value from setlocale(). However, the manual for
setlocale() says:

If locale is "", each part of the locale that should be modified
is set according to the environment variables. The details
are implementation-dependent.

Trivial patch attached.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

The following SQL succeeds:
create database foodb with
template = template0
encoding = 'UTF8'
lc_collate=''
lc_ctype='';

Sure.

Surely we don't want it to be set from the environment, right?

Why not? We have always done that, and in fact the various lc_xxx GUC
variables are documented thusly:

If this variable is set to the empty string (which is the
default) then the value is inherited from the execution
environment of the server in a system-dependent way.

The "trivial patch" you propose breaks that behavior.

I do agree that it's probably unwise to store an empty string as the
value of pg_database.datcollate or datctype, because that would mean
that if the server is restarted with different LC_XXX environment values
then it will think the database has different locale settings, leading
to havoc. However, ISTM the right fix is to replace an empty-string
value with the implied locale name at createdb time. Proposed patch
attached.

Note 1: there's no need to change the behavior for the locale GUCs,
since we don't have any assumptions that those hold still over server
restarts.

Note 2: there is code in initdb that is supposed to be kept parallel
to this, but it's not currently making any attempt to canonicalize
non-empty locale names. Should we make it do that too?

regards, tom lane

On Sat, 2012-03-24 at 19:07 -0400, Tom Lane wrote:

Jeff Davis <pgsql@j-davis.com> writes:

Surely we don't want it to be set from the environment, right?

Why not?

I agree that we shouldn't change the documented behavior of those GUCs.

But a SQL command like CREATE DATABASE being environment sensitive does
seem like surprising behavior to me, and it did indirectly
lead to a bug.

I do agree that it's probably unwise to store an empty string as the
value of pg_database.datcollate or datctype, because that would mean
that if the server is restarted with different LC_XXX environment values
then it will think the database has different locale settings, leading
to havoc.

Yes, that's the worst of the problem. I should have mentioned that more
explicitly in the original report.

However, ISTM the right fix is to replace an empty-string
value with the implied locale name at createdb time. Proposed patch
attached.

+1.

Note 2: there is code in initdb that is supposed to be kept parallel
to this, but it's not currently making any attempt to canonicalize
non-empty locale names. Should we make it do that too?

I assume you are talking about the code that results in writing the
settings to postgresql.conf?

It doesn't look quite as dangerous in that area because (a) it ignores
zero-length strings; and (b) setting the GUC to the wrong value will
either be prevented or will not cause any major problem. However, it
does seem like a good idea to canonicalize the settings unless there is
some reason not to.

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

On Sat, 2012-03-24 at 19:07 -0400, Tom Lane wrote:

Jeff Davis <pgsql@j-davis.com> writes:

Surely we don't want it to be set from the environment, right?

Why not?

I agree that we shouldn't change the documented behavior of those GUCs.
But a SQL command like CREATE DATABASE being environment sensitive does
seem like surprising behavior to me, and it did indirectly
lead to a bug.

Well, our locale documentation makes it pretty clear that a lot of this
behavior is inherited from the server's environment by default:
http://www.postgresql.org/docs/devel/static/locale.html
So I don't see anything wrong with that in principle. But we'd better
make sure that a database's lc_collate/lc_ctype are locked down after
creation.

Note 2: there is code in initdb that is supposed to be kept parallel
to this, but it's not currently making any attempt to canonicalize
non-empty locale names. Should we make it do that too?

I assume you are talking about the code that results in writing the
settings to postgresql.conf?

It doesn't look quite as dangerous in that area because (a) it ignores
zero-length strings; and (b) setting the GUC to the wrong value will
either be prevented or will not cause any major problem. However, it
does seem like a good idea to canonicalize the settings unless there is
some reason not to.

When I wrote the proposed patch, I was imagining that setlocale()
would in fact do some canonicalization of the supplied string.
Experimentation shows that's not so, though, at least not on current
Linux and OS X: you seem to get back the supplied string in all cases
except where it's "".

The reason I was hoping for canonicalization is that right now we
consider locale names like "en_US.utf8" and "en_US.UTF-8" to be
different, even though libc very probably treats them the same;
this results in CREATE DATABASE rejecting lc_collate/ctype settings
that are only cosmetically different from the template database's
values. Canonicalization by setlocale() would fix that without
requiring us to make any unsupported assumptions about which names
mean the same. Oh well.

However, it might still be that somewhere there is a libc that does
take the opportunity to canonicalize the locale name, and if that
did happen then it would be important for initdb to do it the same
way as CREATE DATABASE does. Otherwise, we might end up rejecting
a CREATE DATABASE lc_collate/ctype setting that's identical to what
the user told initdb to use, because one got canonicalized and the
other not. So this roundabout series of assumptions leads me to
think that initdb needs to be tweaked too.

regards, tom lane