Обсуждение: Fix for pg_upgrade's forcing pg_controldata into English
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. +
Index: contrib/pg_upgrade/controldata.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/pg_upgrade/controldata.c,v
retrieving revision 1.9
diff -c -c -r1.9 controldata.c
*** contrib/pg_upgrade/controldata.c 6 Jul 2010 19:18:55 -0000 1.9
--- contrib/pg_upgrade/controldata.c 1 Sep 2010 20:30:45 -0000
***************
*** 11,16 ****
--- 11,17 ----
#include <ctype.h>
+ static void putenv2(migratorContext *ctx, const char *var, const char *val);
/*
* get_control_data()
***************
*** 52,69 ****
bool got_date_is_int = false;
bool got_float8_pass_by_value = false;
char *lang = NULL;
/*
! * 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
snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
cluster->bindir,
live_check ? "pg_controldata\"" : "pg_resetxlog\" -n",
--- 53,82 ----
bool got_date_is_int = false;
bool got_float8_pass_by_value = false;
char *lang = NULL;
+ char *language = NULL;
+ char *lc_messages = NULL;
/*
! * Because we test the pg_resetxlog output as strings, it has to be in
! * English. Copied from pg_regress.c.
*/
if (getenv("LANG"))
lang = pg_strdup(ctx, getenv("LANG"));
+ if (getenv("LANGUAGE"))
+ language = pg_strdup(ctx, getenv("LANGUAGE"));
+ if (getenv("LC_MESSAGES"))
+ lc_messages = pg_strdup(ctx, getenv("LC_MESSAGES"));
+
+ putenv2(ctx, "LANG",
#ifndef WIN32
! NULL);
#else
! /* On Windows the default locale cannot be English, so force it */
! "en");
#endif
+ putenv2(ctx, "LANGUAGE", NULL);
+ putenv2(ctx, "LC_MESSAGES", "C");
+
snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
cluster->bindir,
live_check ? "pg_controldata\"" : "pg_resetxlog\" -n",
***************
*** 334,361 ****
if (output)
pclose(output);
! /* restore LANG */
! if (lang)
! {
! #ifndef WIN32
! char *envstr = (char *) pg_malloc(ctx, strlen(lang) + 6);
!
! sprintf(envstr, "LANG=%s", lang);
! putenv(envstr);
! #else
! SetEnvironmentVariableA("LANG", lang);
! #endif
! pg_free(lang);
! }
! else
! {
! #ifndef WIN32
! unsetenv("LANG");
! #else
! SetEnvironmentVariableA("LANG", "");
! #endif
! }
!
/* verify that we got all the mandatory pg_control data */
if (!got_xid || !got_oid ||
(!live_check && !got_log_id) ||
--- 347,360 ----
if (output)
pclose(output);
! /* restore environment variable settings */
! putenv2(ctx, "LANG", lang);
! putenv2(ctx, "LANGUAGE", language);
! putenv2(ctx, "LC_MESSAGES", lc_messages);
! pg_free(lang);
! pg_free(language);
! pg_free(lc_messages);
!
/* verify that we got all the mandatory pg_control data */
if (!got_xid || !got_oid ||
(!live_check && !got_log_id) ||
***************
*** 492,494 ****
--- 491,524 ----
pg_log(ctx, PG_FATAL, "Unable to rename %s to %s.\n", old_path, new_path);
check_ok(ctx);
}
+
+
+ /*
+ * This is like putenv(), but takes two arguments
+ * It also does unsetenv() if val is NULL.
+ */
+ static void
+ putenv2(migratorContext *ctx, const char *var, const char *val)
+ {
+ if (val)
+ {
+ #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
+ }
+ else
+ {
+ #ifndef WIN32
+ unsetenv(var);
+ #else
+ SetEnvironmentVariableA(var, "");
+ #endif
+ }
+ }
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. + Index: contrib/pg_upgrade/controldata.c =================================================================== RCS file: /cvsroot/pgsql/contrib/pg_upgrade/controldata.c,v retrieving revision 1.9 diff -c -c -r1.9 controldata.c *** contrib/pg_upgrade/controldata.c 6 Jul 2010 19:18:55 -0000 1.9 --- contrib/pg_upgrade/controldata.c 1 Sep 2010 22:38:59 -0000 *************** *** 11,16 **** --- 11,17 ---- #include <ctype.h> + static void putenv2(migratorContext *ctx, const char *var, const char *val); /* * get_control_data() *************** *** 51,69 **** bool got_toast = false; bool got_date_is_int = false; bool got_float8_pass_by_value = false; char *lang = NULL; /* ! * 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 snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE, cluster->bindir, live_check ? "pg_controldata\"" : "pg_resetxlog\" -n", --- 52,106 ---- bool got_toast = false; bool got_date_is_int = false; bool got_float8_pass_by_value = false; + char *lc_collate = NULL; + char *lc_ctype = NULL; + char *lc_monetary = NULL; + char *lc_numeric = NULL; + char *lc_time = NULL; char *lang = NULL; + char *language = NULL; + char *lc_all = NULL; + char *lc_messages = NULL; /* ! * Because we test the pg_resetxlog output as strings, it has to be in ! * English. Copied from pg_regress.c. */ + if (getenv("LC_COLLATE")) + lc_collate = pg_strdup(ctx, getenv("LC_COLLATE")); + if (getenv("LC_CTYPE")) + lc_ctype = pg_strdup(ctx, getenv("LC_CTYPE")); + if (getenv("LC_MONETARY")) + lc_monetary = pg_strdup(ctx, getenv("LC_MONETARY")); + if (getenv("LC_NUMERIC")) + lc_numeric = pg_strdup(ctx, getenv("LC_NUMERIC")); + if (getenv("LC_TIME")) + lc_time = pg_strdup(ctx, getenv("LC_TIME")); if (getenv("LANG")) lang = pg_strdup(ctx, getenv("LANG")); + if (getenv("LANGUAGE")) + language = pg_strdup(ctx, getenv("LANGUAGE")); + if (getenv("LC_ALL")) + lc_all = pg_strdup(ctx, getenv("LC_ALL")); + if (getenv("LC_MESSAGES")) + lc_messages = pg_strdup(ctx, getenv("LC_MESSAGES")); + + putenv2(ctx, "LC_COLLATE", NULL); + putenv2(ctx, "LC_CTYPE", NULL); + putenv2(ctx, "LC_MONETARY", NULL); + putenv2(ctx, "LC_NUMERIC", NULL); + putenv2(ctx, "LC_TIME", NULL); + putenv2(ctx, "LANG", #ifndef WIN32 ! NULL); #else ! /* On Windows the default locale cannot be English, so force it */ ! "en"); #endif + putenv2(ctx, "LANGUAGE", NULL); + putenv2(ctx, "LC_ALL", NULL); + putenv2(ctx, "LC_MESSAGES", "C"); + snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE, cluster->bindir, live_check ? "pg_controldata\"" : "pg_resetxlog\" -n", *************** *** 334,361 **** if (output) pclose(output); ! /* restore LANG */ ! if (lang) ! { ! #ifndef WIN32 ! char *envstr = (char *) pg_malloc(ctx, strlen(lang) + 6); ! ! sprintf(envstr, "LANG=%s", lang); ! putenv(envstr); ! #else ! SetEnvironmentVariableA("LANG", lang); ! #endif ! pg_free(lang); ! } ! else ! { ! #ifndef WIN32 ! unsetenv("LANG"); ! #else ! SetEnvironmentVariableA("LANG", ""); ! #endif ! } ! /* verify that we got all the mandatory pg_control data */ if (!got_xid || !got_oid || (!live_check && !got_log_id) || --- 371,399 ---- if (output) pclose(output); ! /* ! * Restore environment variables ! */ ! putenv2(ctx, "LC_COLLATE", lc_collate); ! putenv2(ctx, "LC_CTYPE", lc_ctype); ! putenv2(ctx, "LC_MONETARY", lc_monetary); ! putenv2(ctx, "LC_NUMERIC", lc_numeric); ! putenv2(ctx, "LC_TIME", lc_time); ! putenv2(ctx, "LANG", lang); ! putenv2(ctx, "LANGUAGE", language); ! putenv2(ctx, "LC_ALL", lc_all); ! putenv2(ctx, "LC_MESSAGES", lc_messages); ! ! pg_free(lc_collate); ! pg_free(lc_ctype); ! pg_free(lc_monetary); ! pg_free(lc_numeric); ! pg_free(lc_time); ! pg_free(lang); ! pg_free(language); ! pg_free(lc_all); ! pg_free(lc_messages); ! /* verify that we got all the mandatory pg_control data */ if (!got_xid || !got_oid || (!live_check && !got_log_id) || *************** *** 492,494 **** --- 530,563 ---- pg_log(ctx, PG_FATAL, "Unable to rename %s to %s.\n", old_path, new_path); check_ok(ctx); } + + + /* + * This is like putenv(), but takes two arguments + * It also does unsetenv() if val is NULL. + */ + static void + putenv2(migratorContext *ctx, const char *var, const char *val) + { + if (val) + { + #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 + } + else + { + #ifndef WIN32 + unsetenv(var); + #else + SetEnvironmentVariableA(var, ""); + #endif + } + }
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. + Index: contrib/pg_upgrade/controldata.c =================================================================== RCS file: /cvsroot/pgsql/contrib/pg_upgrade/controldata.c,v retrieving revision 1.9 diff -c -c -r1.9 controldata.c *** contrib/pg_upgrade/controldata.c 6 Jul 2010 19:18:55 -0000 1.9 --- contrib/pg_upgrade/controldata.c 4 Sep 2010 17:03:41 -0000 *************** *** 11,16 **** --- 11,17 ---- #include <ctype.h> + static void putenv2(migratorContext *ctx, const char *var, const char *val); /* * get_control_data() *************** *** 51,69 **** bool got_toast = false; bool got_date_is_int = false; bool got_float8_pass_by_value = false; char *lang = NULL; /* ! * 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 snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE, cluster->bindir, live_check ? "pg_controldata\"" : "pg_resetxlog\" -n", --- 52,106 ---- bool got_toast = false; bool got_date_is_int = false; bool got_float8_pass_by_value = false; + char *lc_collate = NULL; + char *lc_ctype = NULL; + char *lc_monetary = NULL; + char *lc_numeric = NULL; + char *lc_time = NULL; char *lang = NULL; + char *language = NULL; + char *lc_all = NULL; + char *lc_messages = NULL; /* ! * Because we test the pg_resetxlog output as strings, it has to be in ! * English. Copied from pg_regress.c. */ + if (getenv("LC_COLLATE")) + lc_collate = pg_strdup(ctx, getenv("LC_COLLATE")); + if (getenv("LC_CTYPE")) + lc_ctype = pg_strdup(ctx, getenv("LC_CTYPE")); + if (getenv("LC_MONETARY")) + lc_monetary = pg_strdup(ctx, getenv("LC_MONETARY")); + if (getenv("LC_NUMERIC")) + lc_numeric = pg_strdup(ctx, getenv("LC_NUMERIC")); + if (getenv("LC_TIME")) + lc_time = pg_strdup(ctx, getenv("LC_TIME")); if (getenv("LANG")) lang = pg_strdup(ctx, getenv("LANG")); + if (getenv("LANGUAGE")) + language = pg_strdup(ctx, getenv("LANGUAGE")); + if (getenv("LC_ALL")) + lc_all = pg_strdup(ctx, getenv("LC_ALL")); + if (getenv("LC_MESSAGES")) + lc_messages = pg_strdup(ctx, getenv("LC_MESSAGES")); + + putenv2(ctx, "LC_COLLATE", NULL); + putenv2(ctx, "LC_CTYPE", NULL); + putenv2(ctx, "LC_MONETARY", NULL); + putenv2(ctx, "LC_NUMERIC", NULL); + putenv2(ctx, "LC_TIME", NULL); + putenv2(ctx, "LANG", #ifndef WIN32 ! NULL); #else ! /* On Windows the default locale cannot be English, so force it */ ! "en"); #endif + putenv2(ctx, "LANGUAGE", NULL); + putenv2(ctx, "LC_ALL", NULL); + putenv2(ctx, "LC_MESSAGES", "C"); + snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE, cluster->bindir, live_check ? "pg_controldata\"" : "pg_resetxlog\" -n", *************** *** 334,361 **** if (output) pclose(output); ! /* restore LANG */ ! if (lang) ! { ! #ifndef WIN32 ! char *envstr = (char *) pg_malloc(ctx, strlen(lang) + 6); ! ! sprintf(envstr, "LANG=%s", lang); ! putenv(envstr); ! #else ! SetEnvironmentVariableA("LANG", lang); ! #endif ! pg_free(lang); ! } ! else ! { ! #ifndef WIN32 ! unsetenv("LANG"); ! #else ! SetEnvironmentVariableA("LANG", ""); ! #endif ! } ! /* verify that we got all the mandatory pg_control data */ if (!got_xid || !got_oid || (!live_check && !got_log_id) || --- 371,399 ---- if (output) pclose(output); ! /* ! * Restore environment variables ! */ ! putenv2(ctx, "LC_COLLATE", lc_collate); ! putenv2(ctx, "LC_CTYPE", lc_ctype); ! putenv2(ctx, "LC_MONETARY", lc_monetary); ! putenv2(ctx, "LC_NUMERIC", lc_numeric); ! putenv2(ctx, "LC_TIME", lc_time); ! putenv2(ctx, "LANG", lang); ! putenv2(ctx, "LANGUAGE", language); ! putenv2(ctx, "LC_ALL", lc_all); ! putenv2(ctx, "LC_MESSAGES", lc_messages); ! ! pg_free(lc_collate); ! pg_free(lc_ctype); ! pg_free(lc_monetary); ! pg_free(lc_numeric); ! pg_free(lc_time); ! pg_free(lang); ! pg_free(language); ! pg_free(lc_all); ! pg_free(lc_messages); ! /* verify that we got all the mandatory pg_control data */ if (!got_xid || !got_oid || (!live_check && !got_log_id) || *************** *** 492,494 **** --- 530,568 ---- pg_log(ctx, PG_FATAL, "Unable to rename %s to %s.\n", old_path, new_path); check_ok(ctx); } + + + /* + * putenv2() + * + * This is like putenv(), but takes two arguments. + * It also does unsetenv() if val is NULL. + */ + static void + putenv2(migratorContext *ctx, const char *var, const char *val) + { + if (val) + { + #ifndef WIN32 + char *envstr = (char *) pg_malloc(ctx, strlen(var) + + strlen(val) + 1); + + sprintf(envstr, "%s=%s", var, val); + putenv(envstr); + /* + * Do not free envstr because it becomes part of the environment + * on some operating systems. See port/unsetenv.c::unsetenv. + */ + #else + SetEnvironmentVariableA(var, val); + #endif + } + else + { + #ifndef WIN32 + unsetenv(var); + #else + SetEnvironmentVariableA(var, ""); + #endif + } + } Index: src/port/unsetenv.c =================================================================== RCS file: /cvsroot/pgsql/src/port/unsetenv.c,v retrieving revision 1.11 diff -c -c -r1.11 unsetenv.c *** src/port/unsetenv.c 2 Jan 2010 16:58:13 -0000 1.11 --- src/port/unsetenv.c 4 Sep 2010 17:03:41 -0000 *************** *** 32,37 **** --- 32,38 ---- * implementations (notably recent BSDs) that do not obey SUS but copy the * presented string. This method fails on such platforms. Hopefully all * such platforms have unsetenv() and thus won't be using this hack. + * See: http://www.greenend.org.uk/rjk/2008/putenv.html * * Note that repeatedly setting and unsetting a var using this code will * leak memory.
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. +