Thread
-
Fix for pg_upgrade's forcing pg_controldata into English
Bruce Momjian <bruce@momjian.us> — 2010-09-01T20:35:18Z
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Greg Sabino Mullane wrote: > >> Specifically, LANGUAGE changes the headers of pg_controldata > >> (but not the actual output, LC_ALL does that). Thanks for the > >> nudge, I'll get to rewriting some code. > > > pg_upgrade does this in controldata.c for this exact reason: > > > /* > > * Because we test the pg_resetxlog output strings, it has to be in > > * English. > > */ > > if (getenv("LANG")) > > lang = pg_strdup(ctx, getenv("LANG")); > > #ifndef WIN32 > > putenv(pg_strdup(ctx, "LANG=C")); > > #else > > SetEnvironmentVariableA("LANG", "C"); > > #endif > > You do realize that's far from bulletproof? To be sure that that does > anything, you'd need to set (or unset) LC_ALL and LC_MESSAGES as well. > And I thought Windows spelled it LANGUAGE not LANG ... 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. Should I set LC_ALL too? The other variables mentioned in pg_regress.c? Is this something I should patch to 9.0.X? Patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -
Re: Fix for pg_upgrade's forcing pg_controldata into English
Alvaro Herrera <alvherre@commandprompt.com> — 2010-09-01T20:44:34Z
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
-
Re: Fix for pg_upgrade's forcing pg_controldata into English
Tom Lane <tgl@sss.pgh.pa.us> — 2010-09-01T20:51:06Z
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
-
Re: Fix for pg_upgrade's forcing pg_controldata into English
Bruce Momjian <bruce@momjian.us> — 2010-09-01T22:39:39Z
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. +
-
Re: Fix for pg_upgrade's forcing pg_controldata into English
Tom Lane <tgl@sss.pgh.pa.us> — 2010-09-01T23:16:34Z
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
-
Re: Fix for pg_upgrade's forcing pg_controldata into English
Bruce Momjian <bruce@momjian.us> — 2010-09-01T23:56:45Z
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. +
-
Re: Fix for pg_upgrade's forcing pg_controldata into English
Tom Lane <tgl@sss.pgh.pa.us> — 2010-09-02T00:08:47Z
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 -
Re: Fix for pg_upgrade's forcing pg_controldata into English
Bruce Momjian <bruce@momjian.us> — 2010-09-04T17:50:08Z
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. +
-
Re: Fix for pg_upgrade's forcing pg_controldata into English
Bruce Momjian <bruce@momjian.us> — 2010-09-07T14:11:09Z
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. +