Re: Thread-unsafe coding in ecpg - Mailing list pgsql-hackers

I wrote:
> On the third hand, the lack of previous reports suggests that maybe
> this whole thing is seldom a problem in practice.  Maybe we should
> just use uselocale() where available and otherwise hope it's OK
> to keep on doing what we were doing.

If we go with that approach, I think we need to adapt the patch
as attached.  I autoconfiscated it and fixed a portability problem
(it didn't compile on macOS, which has these decls in <xlocale.h>).

I've verified that this fixes the problem I was seeing on OpenBSD 6.4.
I've not bothered to test on a platform lacking uselocale() --- I
think it's clear by inspection that the patch doesn't change anything
in that case.

Not sure if we need to document this or not.  On platforms with
uselocale(), it should fix the problem without any need for user
attention.  On platforms without, there's no change, and given
the lack of previous complaints I'm not sure it's really an issue.

            regards, tom lane

diff --git a/configure b/configure
index 7602e65..1e69eda 100755
*** a/configure
--- b/configure
*************** fi
*** 15209,15215 ****
  LIBS_including_readline="$LIBS"
  LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`

! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll
posix_fallocateppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul
strsignalsymlink sync_file_range utime utimes wcstombs_l 
  do :
    as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
  ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- 15209,15215 ----
  LIBS_including_readline="$LIBS"
  LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`

! for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll
posix_fallocateppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul
strsignalsymlink sync_file_range uselocale utime utimes wcstombs_l 
  do :
    as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
  ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/configure.in b/configure.in
index d599ad8..556186c 100644
*** a/configure.in
--- b/configure.in
*************** AC_CHECK_FUNCS(m4_normalize([
*** 1618,1623 ****
--- 1618,1624 ----
      strsignal
      symlink
      sync_file_range
+     uselocale
      utime
      utimes
      wcstombs_l
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 9d99816..2c899a1 100644
*** a/src/include/pg_config.h.in
--- b/src/include/pg_config.h.in
***************
*** 691,696 ****
--- 691,699 ----
  /* Define to 1 if the system has the type `unsigned long long int'. */
  #undef HAVE_UNSIGNED_LONG_LONG_INT

+ /* Define to 1 if you have the `uselocale' function. */
+ #undef HAVE_USELOCALE
+
  /* Define to 1 if you have the `utime' function. */
  #undef HAVE_UTIME

diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32
index 8a560ef..3964433 100644
*** a/src/include/pg_config.h.win32
--- b/src/include/pg_config.h.win32
***************
*** 545,550 ****
--- 545,553 ----
  /* Define to 1 if you have the `unsetenv' function. */
  /* #undef HAVE_UNSETENV */

+ /* Define to 1 if you have the `uselocale' function. */
+ /* #undef HAVE_USELOCALE */
+
  /* Define to 1 if you have the `utime' function. */
  #define HAVE_UTIME 1

diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1..22a0557 100644
*** a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
--- b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
***************
*** 12,17 ****
--- 12,20 ----
  #ifndef CHAR_BIT
  #include <limits.h>
  #endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include <xlocale.h>
+ #endif

  enum COMPAT_MODE
  {
*************** struct statement
*** 61,67 ****
--- 64,75 ----
      bool        questionmarks;
      struct variable *inlist;
      struct variable *outlist;
+ #ifdef HAVE_USELOCALE
+     locale_t    clocale;
+     locale_t    oldlocale;
+ #else
      char       *oldlocale;
+ #endif
      int            nparams;
      char      **paramvalues;
      PGresult   *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e..d3e32d2 100644
*** a/src/interfaces/ecpg/ecpglib/execute.c
--- b/src/interfaces/ecpg/ecpglib/execute.c
*************** free_statement(struct statement *stmt)
*** 102,108 ****
--- 102,113 ----
      free_variable(stmt->outlist);
      ecpg_free(stmt->command);
      ecpg_free(stmt->name);
+ #ifdef HAVE_USELOCALE
+     if (stmt->clocale)
+         freelocale(stmt->clocale);
+ #else
      ecpg_free(stmt->oldlocale);
+ #endif
      ecpg_free(stmt);
  }

*************** ecpg_do_prologue(int lineno, const int c
*** 1771,1778 ****

      /*
       * Make sure we do NOT honor the locale for numeric input/output since the
!      * database wants the standard decimal point
       */
      stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
      if (stmt->oldlocale == NULL)
      {
--- 1776,1798 ----

      /*
       * Make sure we do NOT honor the locale for numeric input/output since the
!      * database wants the standard decimal point.  If available, use
!      * uselocale() for this because it's thread-safe.
       */
+ #ifdef HAVE_USELOCALE
+     stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+     if (stmt->clocale == (locale_t) 0)
+     {
+         ecpg_do_epilogue(stmt);
+         return false;
+     }
+     stmt->oldlocale = uselocale(stmt->clocale);
+     if (stmt->oldlocale == (locale_t) 0)
+     {
+         ecpg_do_epilogue(stmt);
+         return false;
+     }
+ #else
      stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
      if (stmt->oldlocale == NULL)
      {
*************** ecpg_do_prologue(int lineno, const int c
*** 1780,1785 ****
--- 1800,1806 ----
          return false;
      }
      setlocale(LC_NUMERIC, "C");
+ #endif

  #ifdef ENABLE_THREAD_SAFETY
      ecpg_pthreads_init();
*************** ecpg_do_epilogue(struct statement *stmt)
*** 1982,1989 ****
--- 2003,2015 ----
      if (stmt == NULL)
          return;

+ #ifdef HAVE_USELOCALE
+     if (stmt->oldlocale)
+         uselocale(stmt->oldlocale);
+ #else
      if (stmt->oldlocale)
          setlocale(LC_NUMERIC, stmt->oldlocale);
+ #endif

      free_statement(stmt);
  }