Re: Fix for pg_upgrade's forcing pg_controldata into English

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.