Fix for pg_upgrade's forcing pg_controldata into English

Excerpts from Bruce Momjian's message of mié sep 01 16:35:18 -0400 2010:

I have implemented your suggestion of setting LANG, LANGUAGE, and
LC_MESSAGES based on code in pg_regress.c; that is the only place I see
where we force English, and it certainly has received more testing.

I think a real long-term solution is to have a machine-readable format
for pg_controldata, perhaps something very simple like

$ pg_controldata --machine
PGCONTROL_VERSION_NUMBER=903
CATALOG_VERSION_NUMBER=201008051
DATABASE_SYSIDENTIFIER=5504177303240039672
etc

This wouldn't be subject to translation and thus much easier to process.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Bruce Momjian's message of mié sep 01 16:35:18 -0400 2010:

I have implemented your suggestion of setting LANG, LANGUAGE, and
LC_MESSAGES based on code in pg_regress.c; that is the only place I see
where we force English, and it certainly has received more testing.

I think a real long-term solution is to have a machine-readable format
for pg_controldata, perhaps something very simple like

$ pg_controldata --machine
PGCONTROL_VERSION_NUMBER=903
CATALOG_VERSION_NUMBER=201008051
DATABASE_SYSIDENTIFIER=5504177303240039672
etc

This wouldn't be subject to translation and thus much easier to process.

+1. pg_controldata was never written with the idea that its output
would be read by programs. If we're going to start doing that, we
should provide an output format that's suitable, not try to work around
it in the callers.

However, that's something for 9.1 and beyond. Bruce's immediate problem
is what to do in pg_upgrade in 9.0, and there I concur that he should
duplicate what pg_regress is doing.

regards, tom lane

Tom Lane wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Bruce Momjian's message of mi�� sep 01 16:35:18 -0400 2010:

I have implemented your suggestion of setting LANG, LANGUAGE, and
LC_MESSAGES based on code in pg_regress.c; that is the only place I see
where we force English, and it certainly has received more testing.

I think a real long-term solution is to have a machine-readable format
for pg_controldata, perhaps something very simple like

$ pg_controldata --machine
PGCONTROL_VERSION_NUMBER=903
CATALOG_VERSION_NUMBER=201008051
DATABASE_SYSIDENTIFIER=5504177303240039672
etc

This wouldn't be subject to translation and thus much easier to process.

+1. pg_controldata was never written with the idea that its output
would be read by programs. If we're going to start doing that, we
should provide an output format that's suitable, not try to work around
it in the callers.

However, that's something for 9.1 and beyond. Bruce's immediate problem
is what to do in pg_upgrade in 9.0, and there I concur that he should
duplicate what pg_regress is doing.

OK, here is a patch that sets all the variables that pg_regress.c sets.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

However, that's something for 9.1 and beyond. Bruce's immediate problem
is what to do in pg_upgrade in 9.0, and there I concur that he should
duplicate what pg_regress is doing.

OK, here is a patch that sets all the variables that pg_regress.c sets.

I certainly hope that pg_regress isn't freeing the strings it passes
to putenv() ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

However, that's something for 9.1 and beyond. Bruce's immediate problem
is what to do in pg_upgrade in 9.0, and there I concur that he should
duplicate what pg_regress is doing.

OK, here is a patch that sets all the variables that pg_regress.c sets.

I certainly hope that pg_regress isn't freeing the strings it passes
to putenv() ...

pg_regress does not restore these settings (it says with C/English) so
the code is different.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

I certainly hope that pg_regress isn't freeing the strings it passes
to putenv() ...

pg_regress does not restore these settings (it says with C/English) so
the code is different.

That's not what I'm on about. You're trashing strings that are part of
the live environment. It might accidentally fail to fail for you, if
your version of free() doesn't immediately clobber the released storage,
but it's still broken. Read the putenv() man page.

+ #ifndef WIN32
+ 		char	   *envstr = (char *) pg_malloc(ctx, strlen(var) +
+ 							strlen(val) + 1);
+ 
+ 		sprintf(envstr, "%s=%s", var, val);
+ 		putenv(envstr);
+ 		pg_free(envstr);
                ^^^^^^^^^^^^^^^^
+ #else
+ 		SetEnvironmentVariableA(var, val);
+ #endif

The fact that there is no such free() in pg_regress is not an oversight
or shortcut.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

I certainly hope that pg_regress isn't freeing the strings it passes
to putenv() ...

pg_regress does not restore these settings (it says with C/English) so
the code is different.

That's not what I'm on about. You're trashing strings that are part of
the live environment. It might accidentally fail to fail for you, if
your version of free() doesn't immediately clobber the released storage,
but it's still broken. Read the putenv() man page.

+ #ifndef WIN32
+ 		char	   *envstr = (char *) pg_malloc(ctx, strlen(var) +
+ 							strlen(val) + 1);
+ 
+ 		sprintf(envstr, "%s=%s", var, val);
+ 		putenv(envstr);
+ 		pg_free(envstr);
^^^^^^^^^^^^^^^^
+ #else
+ 		SetEnvironmentVariableA(var, val);
+ #endif

The fact that there is no such free() in pg_regress is not an oversight
or shortcut.

Interesting. I did not know this and it was not clear from my manual
page or FreeBSD's manual page, but Linux clearly does this.

Updated patch attached.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

I certainly hope that pg_regress isn't freeing the strings it passes
to putenv() ...

pg_regress does not restore these settings (it says with C/English) so
the code is different.

That's not what I'm on about. You're trashing strings that are part of
the live environment. It might accidentally fail to fail for you, if
your version of free() doesn't immediately clobber the released storage,
but it's still broken. Read the putenv() man page.

+ #ifndef WIN32
+ 		char	   *envstr = (char *) pg_malloc(ctx, strlen(var) +
+ 							strlen(val) + 1);
+ 
+ 		sprintf(envstr, "%s=%s", var, val);
+ 		putenv(envstr);
+ 		pg_free(envstr);
^^^^^^^^^^^^^^^^
+ #else
+ 		SetEnvironmentVariableA(var, val);
+ #endif

The fact that there is no such free() in pg_regress is not an oversight
or shortcut.

Interesting. I did not know this and it was not clear from my manual
page or FreeBSD's manual page, but Linux clearly does this.

Updated patch attached.

Applied to HEAD and 9.0.X. Thanks for the ideas/review.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +