Re: Thread-unsafe coding in ecpg
> I like having a hard limit on the number of loop iterations; > that should ensure that the test terminates no matter how confused > ecpglib is. I get your point and thus will only clean up the tests a little bit. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: Thread-unsafe coding in ecpg
Michael Meskes writes: > I wonder if it would be better to make the test cases use the proper > whenever command instead. That would give us a slightly better > functionality testing I'd say. I like having a hard limit on the number of loop iterations; that should ensure that the test terminates no matter how confused ecpglib is. But certainly we could have more of the tests using "whenever" as the intended method of getting out of the loop. regards, tom lane
Re: Thread-unsafe coding in ecpg
> It looks to me like the reason that the ecpg tests went into an > infinite > loop is that compat_informix/test_informix.pgc has not considered the > possibility of repeated statement failures: > ... Correct, this was missing a safeguard. > I know zip about ecpg coding practices, but wouldn't it be a better > idea > to break out of the loop on seeing an error? I wonder if it would be better to make the test cases use the proper whenever command instead. That would give us a slightly better functionality testing I'd say. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: Thread-unsafe coding in ecpg
On 1/23/19 10:53 PM, Tom Lane wrote: > I wrote: >> This suggests that, rather than throwing up our hands if the initial >> _configthreadlocale call returns -1, we should act as though the function >> doesn't exist, and just soldier on the same as before. The code in there >> assumes that -1 is a can't-happen case and doesn't try to recover, >> but apparently that's over-optimistic. > I pushed a patch to fix that. jacana has been upgraded to gcc 8.1.0 so it knows about _configthreadlocale() and it's now happy. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Thread-unsafe coding in ecpg
Oh, I just noticed something else: several of the ecpg test programs contain #ifdef WIN32 #ifdef _MSC_VER/* requires MSVC */ _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); #endif #endif Surely this is a kluge that we could now remove? We've protected the only setlocale calls that the ecpg tests could reach in threaded mode. If there is still a problem, it'd behoove us to find it, because ecpglib should not expect that the calling app has done this for it. regards, tom lane
Re: Thread-unsafe coding in ecpg
I wrote:
> This suggests that, rather than throwing up our hands if the initial
> _configthreadlocale call returns -1, we should act as though the function
> doesn't exist, and just soldier on the same as before. The code in there
> assumes that -1 is a can't-happen case and doesn't try to recover,
> but apparently that's over-optimistic.
I pushed a patch to fix that.
It looks to me like the reason that the ecpg tests went into an infinite
loop is that compat_informix/test_informix.pgc has not considered the
possibility of repeated statement failures:
while (1)
{
$fetch forward c into :i, :j, :c;
if (sqlca.sqlcode == 100) break;
else if (sqlca.sqlcode != 0) printf ("Error: %ld\n", sqlca.sqlcode);
if (risnull(CDECIMALTYPE, (char *)))
printf("%d NULL\n", i);
else
{
int a;
dectoint(, );
printf("%d %d \"%s\"\n", i, a, c);
}
}
I know zip about ecpg coding practices, but wouldn't it be a better idea
to break out of the loop on seeing an error?
regards, tom lane
Re: Thread-unsafe coding in ecpg
Andrew Dunstan writes: > On 1/23/19 6:01 PM, Tom Lane wrote: >> Perhaps there's some sort of setup that MinGW's version needs that >> we're not doing? > This seems to suggest something worse: https://reviews.llvm.org/D40181 > Not sure I fully understand what's happening here, though. Me either, but I noted a couple of interesting extracts from that page: Normal mingw that uses msvcrt.dll doesn't have per-thread locales so it won't really work in any case (but I think it does define some sort of dummy functions that at least will allow it to build). Nowadays, mingw can be built to target ucrtbase.dll as well though, and there it should be possible to make it work just like for MSVC although it might need some patches. ... Looked into MinGW-w64 sources and this is indeed the case. _configthreadlocale will return -1 and will not do anything. This suggests that, rather than throwing up our hands if the initial _configthreadlocale call returns -1, we should act as though the function doesn't exist, and just soldier on the same as before. The code in there assumes that -1 is a can't-happen case and doesn't try to recover, but apparently that's over-optimistic. regards, tom lane
Re: Thread-unsafe coding in ecpg
On 1/23/19 6:01 PM, Tom Lane wrote: > Andrew Dunstan writes: >> I have just spent a large amount of time testing the committed fix with >> a number of versions of gcc. It blows up on any compiler modern enough >> to know about _configthreadlocale > Bleah. Since the regular Windows buildfarm members seem happy, this > evidently means that MinGW's _configthreadlocale is broken in some way. > > I suppose we could just remove the autoconf test and build without > _configthreadlocale on MinGW, but that's kind of sad ... > > Perhaps there's some sort of setup that MinGW's version needs that > we're not doing? This seems to suggest something worse: https://reviews.llvm.org/D40181 Not sure I fully understand what's happening here, though. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Thread-unsafe coding in ecpg
Andrew Dunstan writes: > I have just spent a large amount of time testing the committed fix with > a number of versions of gcc. It blows up on any compiler modern enough > to know about _configthreadlocale Bleah. Since the regular Windows buildfarm members seem happy, this evidently means that MinGW's _configthreadlocale is broken in some way. I suppose we could just remove the autoconf test and build without _configthreadlocale on MinGW, but that's kind of sad ... Perhaps there's some sort of setup that MinGW's version needs that we're not doing? regards, tom lane
Re: Thread-unsafe coding in ecpg
On 1/22/19 12:50 PM, Andrew Dunstan wrote: > On 1/21/19 10:00 PM, Andrew Dunstan wrote: >> On 1/21/19 3:25 PM, Tom Lane wrote: >>> "Joshua D. Drake" writes: On 1/21/19 12:05 PM, Tom Lane wrote: > Is there a newer version of mingw that does have this functionality? Apparently this can be done with thee 64bit version: https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw >>> Hmm, the followup question makes it sound like it still didn't work :-(. >>> >>> However, since the mingw build is autoconf-based, seems like we can >>> install a configure check instead of guessing. Will make it so. >>> >>> Task for somebody else: run a MinGW64 buildfarm member. >>> >>> >> I could set that up in just about two shakes of a lamb's tail - I have a >> script to do so all tested on vagrant/aws within the last few months. >> >> >> What I don't have is resources. My Windows resources are pretty much >> tapped out. I would need either some modest hardware or a Windows VM >> somewhere in the cloud - could be anywhere but I'm most at home on AWS. >> > > Incidentally, jacana *is* running mingw64, but possibly not a young > enough version. I just ran a test on an up to date version and it found > the config setting happily, and passed the make stage. > > > I can look at upgrading jacana to a more modern compiler. The version > it's running is 4.9.1, apparently built around Oct 2014, so it's not > that old. > > I have just spent a large amount of time testing the committed fix with a number of versions of gcc. It blows up on any compiler modern enough to know about _configthreadlocale Essentially what happens is this: == running regression test queries == test compat_informix/dec_test ... ok test compat_informix/charfuncs ... ok test compat_informix/rfmtdate ... ok test compat_informix/rfmtlong ... ok test compat_informix/rnull ... stdout stderr FAILED test compat_informix/sqlda ... stdout stderr FAILED (test process exited with exit code 1) test compat_informix/describe ... stdout stderr FAILED (test process exited with exit code 1) test compat_informix/test_informix ... At this point the regression tests hang and the disk starts filling up rapidly. This has been tested with versions 5.4.0, 6.4.0, 7.3.0, 8.1.0 and 8.2.1. The first 4 are downloaded direct from the Mingw64 repo, the last is from Msys2's mingw64 toolchain. I'm trying to find out more info, but what we have right now is pretty broken. Reverting commits ee27584c4a and 8eb4a9312 cures[*] the problem. cheers andrew [*] FSVO "cure". I am still getting intermittent errors but no hangs. -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Thread-unsafe coding in ecpg
On January 22, 2019 9:50:19 AM PST, Andrew Dunstan wrote: > >On 1/21/19 10:00 PM, Andrew Dunstan wrote: >> On 1/21/19 3:25 PM, Tom Lane wrote: >>> "Joshua D. Drake" writes: On 1/21/19 12:05 PM, Tom Lane wrote: > Is there a newer version of mingw that does have this >functionality? Apparently this can be done with thee 64bit version: >https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw >>> Hmm, the followup question makes it sound like it still didn't work >:-(. >>> >>> However, since the mingw build is autoconf-based, seems like we can >>> install a configure check instead of guessing. Will make it so. >>> >>> Task for somebody else: run a MinGW64 buildfarm member. >>> >>> >> >> I could set that up in just about two shakes of a lamb's tail - I >have a >> script to do so all tested on vagrant/aws within the last few months. >> >> >> What I don't have is resources. My Windows resources are pretty much >> tapped out. I would need either some modest hardware or a Windows VM >> somewhere in the cloud - could be anywhere but I'm most at home on >AWS. >> > > >Incidentally, jacana *is* running mingw64, but possibly not a young >enough version. I just ran a test on an up to date version and it found >the config setting happily, and passed the make stage. > > >I can look at upgrading jacana to a more modern compiler. The version >it's running is 4.9.1, apparently built around Oct 2014, so it's not >that old. The thread about the introduction is from 2016, so that's kind of expected. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: Thread-unsafe coding in ecpg
On 1/21/19 10:00 PM, Andrew Dunstan wrote: > On 1/21/19 3:25 PM, Tom Lane wrote: >> "Joshua D. Drake" writes: >>> On 1/21/19 12:05 PM, Tom Lane wrote: Is there a newer version of mingw that does have this functionality? >>> Apparently this can be done with thee 64bit version: >>> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw >> Hmm, the followup question makes it sound like it still didn't work :-(. >> >> However, since the mingw build is autoconf-based, seems like we can >> install a configure check instead of guessing. Will make it so. >> >> Task for somebody else: run a MinGW64 buildfarm member. >> >> > > I could set that up in just about two shakes of a lamb's tail - I have a > script to do so all tested on vagrant/aws within the last few months. > > > What I don't have is resources. My Windows resources are pretty much > tapped out. I would need either some modest hardware or a Windows VM > somewhere in the cloud - could be anywhere but I'm most at home on AWS. > Incidentally, jacana *is* running mingw64, but possibly not a young enough version. I just ran a test on an up to date version and it found the config setting happily, and passed the make stage. I can look at upgrading jacana to a more modern compiler. The version it's running is 4.9.1, apparently built around Oct 2014, so it's not that old. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
RE: Thread-unsafe coding in ecpg
From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> BTW, I found another spot in descriptor.c where ecpglib is using
> setlocale() for the same purpose. Perhaps that one's not reachable
> in threaded apps, but I didn't see any obvious reason to think so,
> so I changed it too.
Ouch, thanks. And I'm sorry to annoy you by pointing out a trivial thing: in
v3 patch, _configthreadlocale() is not called to restore the original value
when setlocale() or ecpg_strdup() fails. I hope this is fixed in v4.
+ #ifdef WIN32
+ stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+ if (stmt->oldthreadlocale == -1)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ #endif
stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
if (stmt->oldlocale == NULL)
{
Regards
Takayuki Tsunakawa
Re: Thread-unsafe coding in ecpg
On 1/21/19 3:25 PM, Tom Lane wrote: > "Joshua D. Drake" writes: >> On 1/21/19 12:05 PM, Tom Lane wrote: >>> Is there a newer version of mingw that does have this functionality? >> Apparently this can be done with thee 64bit version: >> https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw > Hmm, the followup question makes it sound like it still didn't work :-(. > > However, since the mingw build is autoconf-based, seems like we can > install a configure check instead of guessing. Will make it so. > > Task for somebody else: run a MinGW64 buildfarm member. > > I could set that up in just about two shakes of a lamb's tail - I have a script to do so all tested on vagrant/aws within the last few months. What I don't have is resources. My Windows resources are pretty much tapped out. I would need either some modest hardware or a Windows VM somewhere in the cloud - could be anywhere but I'm most at home on AWS. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Thread-unsafe coding in ecpg
Andres Freund writes: > We could just refuse to support thread safety on mingw if that's not > supported? Or is that too aggressive? Nah, we already had that discussion upthread. Given the lack of prior complaints, we shouldn't break cases that are working today. For instance, as long as setlocale() isn't actively thread-unsafe and you are running with LC_NUMERIC=C as the prevailing locale, the existing code doesn't pose a hazard even in a threaded app. Forcing users for whom that's true to --disable-thread-safety would turn an OK situation into a broken one. regards, tom lane
Re: Thread-unsafe coding in ecpg
"Joshua D. Drake" writes: > On 1/21/19 12:05 PM, Tom Lane wrote: >> Is there a newer version of mingw that does have this functionality? > Apparently this can be done with thee 64bit version: > https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw Hmm, the followup question makes it sound like it still didn't work :-(. However, since the mingw build is autoconf-based, seems like we can install a configure check instead of guessing. Will make it so. Task for somebody else: run a MinGW64 buildfarm member. regards, tom lane
Re: Thread-unsafe coding in ecpg
Hi, On 2019-01-21 15:05:23 -0500, Tom Lane wrote: > Andres Freund writes: > > Seems jacana might not have like this change? > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 > > Hmm. So mingw doesn't provide access to _configthreadlocale(). > That's unfortunate, at least if we think that mingw is still a viable > production platform, because it means we can't make ecpg thread-safe > on that platform. > > Is there a newer version of mingw that does have this functionality? > I'm not sure whether to install a version check or just assume that > it's never there. It does seem like newer versions do have it: https://sourceforge.net/p/mingw-w64/mailman/message/34765722/ https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/misc/_configthreadlocale.c We could just refuse to support thread safety on mingw if that's not supported? Or is that too aggressive? Greetings, Andres Freund
Re: Thread-unsafe coding in ecpg
On 1/21/19 12:05 PM, Tom Lane wrote: Andres Freund writes: Seems jacana might not have like this change? https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 Hmm. So mingw doesn't provide access to _configthreadlocale(). That's unfortunate, at least if we think that mingw is still a viable production platform, because it means we can't make ecpg thread-safe on that platform. Is there a newer version of mingw that does have this functionality? I'm not sure whether to install a version check or just assume that it's never there. Apparently this can be done with thee 64bit version: https://stackoverflow.com/questions/33647271/how-to-use-configthreadlocale-in-mingw JD regards, tom lane -- Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc PostgreSQL centered full stack support, consulting and development. Advocate: @amplifypostgres || Learn and Network: https://postgresconf.org *** A fault and talent of mine is to tell it exactly how it is. ***
Re: Thread-unsafe coding in ecpg
Andres Freund writes: > Seems jacana might not have like this change? > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 Hmm. So mingw doesn't provide access to _configthreadlocale(). That's unfortunate, at least if we think that mingw is still a viable production platform, because it means we can't make ecpg thread-safe on that platform. Is there a newer version of mingw that does have this functionality? I'm not sure whether to install a version check or just assume that it's never there. regards, tom lane
Re: Thread-unsafe coding in ecpg
On 2019-01-21 12:09:30 -0500, Tom Lane wrote: > "Tsunakawa, Takayuki" writes: > > From: Tom Lane [mailto:t...@sss.pgh.pa.us] > >> So like this ... > > > How quick! Thank you. I've reviewed the code for both Unix and Windows, > > and it looks OK. I haven't built the patch, but expect the buildfarm to do > > the test. > > Thanks for reviewing! I've pushed this now (to HEAD only for the moment), > we'll see what the buildfarm thinks. > > BTW, I found another spot in descriptor.c where ecpglib is using > setlocale() for the same purpose. Perhaps that one's not reachable > in threaded apps, but I didn't see any obvious reason to think so, > so I changed it too. Seems jacana might not have like this change? https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2019-01-21%2019%3A01%3A28 Greetings, Andres Freund
Re: Thread-unsafe coding in ecpg
> Thanks for reviewing! I've pushed this now (to HEAD only for the > moment), > we'll see what the buildfarm thinks. > > BTW, I found another spot in descriptor.c where ecpglib is using > setlocale() for the same purpose. Perhaps that one's not reachable > in threaded apps, but I didn't see any obvious reason to think so, > so I changed it too. Thanks Tom. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: Thread-unsafe coding in ecpg
"Tsunakawa, Takayuki" writes: > From: Tom Lane [mailto:t...@sss.pgh.pa.us] >> So like this ... > How quick! Thank you. I've reviewed the code for both Unix and Windows, and > it looks OK. I haven't built the patch, but expect the buildfarm to do the > test. Thanks for reviewing! I've pushed this now (to HEAD only for the moment), we'll see what the buildfarm thinks. BTW, I found another spot in descriptor.c where ecpglib is using setlocale() for the same purpose. Perhaps that one's not reachable in threaded apps, but I didn't see any obvious reason to think so, so I changed it too. regards, tom lane
RE: Thread-unsafe coding in ecpg
From: Tom Lane [mailto:t...@sss.pgh.pa.us] > Hm. Well, I suppose we can figure that the buildfarm should tell us if > there's anything too old that we still care about. > > So like this ... How quick! Thank you. I've reviewed the code for both Unix and Windows, and it looks OK. I haven't built the patch, but expect the buildfarm to do the test. Regards Takayuki Tsunakawa
Re: Thread-unsafe coding in ecpg
"Tsunakawa, Takayuki" writes:
> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
>> How far back does that exist?
> I couldn't find the relevant doc, but I've just confirmed I can use it with
> Visual Studio 2008 on Win7, which is my oldest combination at hand. VS 2008
> is already past its EOL, and the support for Win7 will end next year, so the
> combination is practically enough.
Hm. Well, I suppose we can figure that the buildfarm should tell us
if there's anything too old that we still care about.
So like this ...
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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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..41851d5 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
#endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include
+ #endif
enum COMPAT_MODE
{
*** struct statement
*** 61,67
--- 64,78
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
+ #ifdef HAVE_USELOCALE
+ locale_t clocale;
+ locale_t oldlocale;
+ #else
char *oldlocale;
+ #ifdef WIN32
+ int oldthreadlocale;
+ #endif
+ #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..f67d774 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,1806
/*
* 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
RE: Thread-unsafe coding in ecpg
From: Tom Lane [mailto:t...@sss.pgh.pa.us] > "Tsunakawa, Takayuki" writes: > > On Windows, _configthreadlocale() enables us to restrict the effect of > setlocale() only to the calling thread. We can call it in > ecpg_do_prolog/epilog(). > > How far back does that exist? I couldn't find the relevant doc, but I've just confirmed I can use it with Visual Studio 2008 on Win7, which is my oldest combination at hand. VS 2008 is already past its EOL, and the support for Win7 will end next year, so the combination is practically enough. Regards Takayuki Tsunakawa
Re: Thread-unsafe coding in ecpg
"Tsunakawa, Takayuki" writes: > On Windows, _configthreadlocale() enables us to restrict the effect of > setlocale() only to the calling thread. We can call it in > ecpg_do_prolog/epilog(). How far back does that exist? regards, tom lane
RE: Thread-unsafe coding in ecpg
On Windows, _configthreadlocale() enables us to restrict the effect of setlocale() only to the calling thread. We can call it in ecpg_do_prolog/epilog(). https://docs.microsoft.com/en-us/cpp/parallel/multithreading-and-locales?view=vs-2017 Regards Takayuki Tsunakawa
Re: Thread-unsafe coding in ecpg
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 ).
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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink 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
#endif
+ #ifdef LOCALE_T_IN_XLOCALE
+ #include
+ #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)
Re: Thread-unsafe coding in ecpg
Michael Meskes writes: >> No, we shouldn't use setlocale(), because it clearly is hazardous >> even on platforms where it doesn't fail outright. I don't see >> anything so wrong with just documenting the hazard. The situation > Actually I meant using setlocale() and documenting that it must not be > used with threads, or document it must not be used with locales? I tend to think that has more downside than upside, in situations where people don't read the manual closely and try to do it anyway. First, there's the probable crash if setlocale() is thread-unsafe. (Though the lack of previous reports suggests that on most platforms, it isn't.) Second, if the program is indeed trying to run with non-C LC_NUMERIC, using setlocale() will have unsynchronized, hard-to-debug side effects on other threads. Not using it will have no downside at all if ecpg isn't trying to read numeric data, while if it does do so, the failures will be reproducible and easy to understand/debug. Admittedly, removing the setlocale() call will be a net negative for single-threaded applications, which are likely the majority. But I don't know any safe way to tell whether the app is multi threaded. 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. regards, tom lane
Re: Thread-unsafe coding in ecpg
> > The question is, what do we do on those platforms? Use setlocale() > > or > > fallback to (a) and document that ecpg has to run in a C locale? > > No, we shouldn't use setlocale(), because it clearly is hazardous > even on platforms where it doesn't fail outright. I don't see > anything so wrong with just documenting the hazard. The situation Actually I meant using setlocale() and documenting that it must not be used with threads, or document it must not be used with locales? Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: Thread-unsafe coding in ecpg
Andrew Gierth writes: > Would it help if we had non-locale-aware functions for both > floating-point output _and_ input? i.e. import a known-working strtod() > (allowing us to remove all the hacks that have grown up around it, for > special-case input and wonky error handling) with locale functionality > removed. Dunno, is there such a thing as a platform-independent strtod()? I'd have thought that, for instance, typical implementations would be pretty much in bed with the details of IEEE float format --- your example where strtof() is different from (float) strtod() makes it hard to believe that it can be written without assumptions about the hardware's float format. (Note that this concern is independent of whether we adopt the Ryu code, which IIUC also depends on IEEE floats. Our answer for anyone wanting to run on non-IEEE hardware can be to #ifdef out Ryu and use the existing float output code. But doing the equivalent thing on the input side wouldn't solve ecpg's problem.) regards, tom lane
Re: Thread-unsafe coding in ecpg
> "Michael" == Michael Meskes writes: >> Therefore, it's plain crazy for ecpg to be calling setlocale() >> inside threaded code. It looks to me like what ecpg is doing is >> trying to defend itself against non-C LC_NUMERIC settings, which is >> laudable, but this implementation of that is totally unsafe. >> >> Don't know what's the best way out of this. The simplest thing would >> be to just remove that code and document that you'd better run ecpg >> in LC_NUMERIC locale, but it'd be nice if we could do better. Would it help if we had non-locale-aware functions for both floating-point output _and_ input? i.e. import a known-working strtod() (allowing us to remove all the hacks that have grown up around it, for special-case input and wonky error handling) with locale functionality removed. -- Andrew (irc:RhodiumToad)
Re: Thread-unsafe coding in ecpg
Michael Meskes writes: >> While (b) has more theoretical purity, I'm inclined to think it >> doesn't really improve anybody's life compared to (a), because >> --disable-thread-safety doesn't actually stop anyone from using >> libpq or ecpglib in threaded environments. It just makes it >> more likely to fail when they do. > The question is, what do we do on those platforms? Use setlocale() or > fallback to (a) and document that ecpg has to run in a C locale? No, we shouldn't use setlocale(), because it clearly is hazardous even on platforms where it doesn't fail outright. I don't see anything so wrong with just documenting the hazard. The situation isn't noticeably more dangerous than simple use of the C library; sscanf, strtod, etc are all likely to do surprising things when LC_NUMERIC isn't C. > We could also rewrite the parsing of numbers to not be locale > dependent. Perhaps, but that seems like a giant undertaking. I'm not excited about duplicating strtod(), for instance. regards, tom lane
Re: Thread-unsafe coding in ecpg
> While (b) has more theoretical purity, I'm inclined to think it > doesn't really improve anybody's life compared to (a), because > --disable-thread-safety doesn't actually stop anyone from using > libpq or ecpglib in threaded environments. It just makes it > more likely to fail when they do. The question is, what do we do on those platforms? Use setlocale() or fallback to (a) and document that ecpg has to run in a C locale? We could also rewrite the parsing of numbers to not be locale dependent. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: Thread-unsafe coding in ecpg
Michael Meskes writes: >> IOW, not only is setlocale() not necessarily thread-safe itself, >> but using it to change locales in a multithread program is seriously >> unsafe because of concurrent effects on other threads. > Agreed. > How about attached patch? According to my manpages it should only > affect the calling threat. I only tested it on my own system so far. > Could you please have a look and/or test on other systems? Yeah, I was wondering about uselocale() myself. We cannot assume it's available everywhere, but it should fix the problem where available. On machines that don't have it, we could either (a) have ecpg do nothing, and just hope you're not using a dangerous locale; or (b) consider the platform not thread-safe, forcing people to specify --disable-thread-safety to build. While (b) has more theoretical purity, I'm inclined to think it doesn't really improve anybody's life compared to (a), because --disable-thread-safety doesn't actually stop anyone from using libpq or ecpglib in threaded environments. It just makes it more likely to fail when they do. The OpenBSD 6.4 platform where I found this problem has uselocale (but the man page notes they only added it as of 6.2). I can test out the patch there, but I think the interesting questions are all about what to do on platforms without the function. regards, tom lane
Re: Thread-unsafe coding in ecpg
> So my conclusion is that this version of setlocale() has some
> thread-safety issues. I was all set to go file a bug report
> when I noticed this in the POSIX spec for setlocale:
>
> The setlocale() function need not be thread-safe.
>
> as well as this:
>
> The global locale established using setlocale() shall only be
> used
> in threads for which no current locale has been set using
> uselocale() or whose current locale has been set to the global
> locale using uselocale(LC_GLOBAL_LOCALE).
This one was new to me.
> IOW, not only is setlocale() not necessarily thread-safe itself,
> but using it to change locales in a multithread program is seriously
> unsafe because of concurrent effects on other threads.
Agreed.
> Therefore, it's plain crazy for ecpg to be calling setlocale() inside
> threaded code. It looks to me like what ecpg is doing is trying to
> defend
> itself against non-C LC_NUMERIC settings, which is laudable, but this
> implementation of that is totally unsafe.
>
> Don't know what's the best way out of this. The simplest thing would
> be to just remove that code and document that you'd better run ecpg
> in LC_NUMERIC locale, but it'd be nice if we could do better.
How about attached patch? According to my manpages it should only
affect the calling threat. I only tested it on my own system so far.
Could you please have a look and/or test on other systems?
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1c9bce1456..92ff7126d9 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -61,7 +61,8 @@ struct statement
bool questionmarks;
struct variable *inlist;
struct variable *outlist;
- char *oldlocale;
+ locale_t clocale;
+ locale_t oldlocale;
int nparams;
char **paramvalues;
PGresult *results;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 3f5034e792..ba03607d5c 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -102,7 +102,8 @@ free_statement(struct statement *stmt)
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
- ecpg_free(stmt->oldlocale);
+ if (stmt->clocale)
+ freelocale(stmt->clocale);
ecpg_free(stmt);
}
@@ -1773,13 +1774,18 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
* 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)
+ 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;
}
- setlocale(LC_NUMERIC, "C");
#ifdef ENABLE_THREAD_SAFETY
ecpg_pthreads_init();
@@ -1983,7 +1989,7 @@ ecpg_do_epilogue(struct statement *stmt)
return;
if (stmt->oldlocale)
- setlocale(LC_NUMERIC, stmt->oldlocale);
+ uselocale(stmt->oldlocale);
free_statement(stmt);
}
Thread-unsafe coding in ecpg
I've found that a couple of different OpenBSD 6.4 machines fall over badly in the ecpg regression tests, with output like test sql/parser ... ok test thread/thread... stdout stderr FAILED (test process was terminated by signal 6: Abort trap) test thread/thread_implicit ... stdout FAILED (test process was terminated by signal 10: Bus error) test thread/prep ... ok (test process was terminated by signal 10: Bus error) test thread/alloc ... stderr FAILED (test process was terminated by signal 6: Abort trap) test thread/descriptor... ok It's somewhat variable as to which tests fail, but it's always thread tests. Examining the core dumps shows traces like #0 thrkill () at -:3 #1 0x0c04f427dd6e in _libc_abort () at /usr/src/lib/libc/stdlib/abort.c:51 #2 0x0c04f425f7e9 in wrterror (d=Variable "d" is not available. ) at /usr/src/lib/libc/stdlib/malloc.c:291 #3 0x0c04f42628fb in find_chunknum (d=Variable "d" is not available. ) at /usr/src/lib/libc/stdlib/malloc.c:1043 #4 0x0c04f425fe23 in ofree (argpool=Variable "argpool" is not available. ) at /usr/src/lib/libc/stdlib/malloc.c:1359 #5 0x0c04f425f8ec in free (ptr=0xc04df0e26e0) at /usr/src/lib/libc/stdlib/malloc.c:1419 #6 0x0c04f427ec83 in freegl (oldgl=0xc05a022d080) at /usr/src/lib/libc/locale/setlocale.c:32 #7 0x0c04f427eb49 in _libc_setlocale (category=4, locname=0xc059b605180 "C") at /usr/src/lib/libc/locale/setlocale.c:177 #8 0x0c0531a6f955 in ecpg_do_epilogue (stmt=0xc0587bb0c00) at execute.c:1986 #9 0x0c0531a6fa65 in ecpg_do (lineno=Variable "lineno" is not available. ) at execute.c:2018 #10 0x0c0531a6fb31 in ECPGdo (lineno=Variable "lineno" is not available. ) at execute.c:2037 #11 0x0c02a9f00b19 in test_thread (arg=Variable "arg" is not available. ) at thread.pgc:131 #12 0x0c04b180b26e in _rthread_start (v=Variable "v" is not available. ) at /usr/src/lib/librthread/rthread.c:96 #13 0x0c04f42ba77b in __tfork_thread () at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:75 #14 0x in ?? () or this one: #0 _libc_strlcpy (dst=0x2c61e133ee0 "C", src=0xdfdfdfdfdfdfdfdf , dsize=256) at /usr/src/lib/libc/string/strlcpy.c:36 #1 0x02c61de99a71 in _libc_setlocale (category=4, locname=0x0) from /usr/lib/libc.so.92.5 #2 0x02c5ae2693a8 in ecpg_do_prologue (lineno=59, compat=0, force_indicator=1, connection_name=0x0, questionmarks=false, statement_type=ECPGst_execute, query=0x2c333701418 "i", args=0x2c61a96f6d0, stmt_out=0x2c61a96f5e0) at execute.c:1776 #3 0x02c5ae269a20 in ecpg_do (lineno=Variable "lineno" is not available. ) at execute.c:2001 #4 0x02c5ae269b31 in ECPGdo (lineno=Variable "lineno" is not available. ) at execute.c:2037 #5 0x02c333600b47 in fn (arg=Variable "arg" is not available. ) at prep.pgc:59 #6 0x02c56a00b26e in _rthread_start (v=Variable "v" is not available. ) at /usr/src/lib/librthread/rthread.c:96 #7 0x02c61ded577b in __tfork_thread () at /usr/src/lib/libc/arch/amd64/sys/tfork_thread.S:75 #8 0x in ?? () The common denominator is always a call to setlocale(), and that generally is calling malloc or free or some other libc function that is unhappy. When output appears on stderr, it's usually free complaining about double-frees or some such. So my conclusion is that this version of setlocale() has some thread-safety issues. I was all set to go file a bug report when I noticed this in the POSIX spec for setlocale: The setlocale() function need not be thread-safe. as well as this: The global locale established using setlocale() shall only be used in threads for which no current locale has been set using uselocale() or whose current locale has been set to the global locale using uselocale(LC_GLOBAL_LOCALE). IOW, not only is setlocale() not necessarily thread-safe itself, but using it to change locales in a multithread program is seriously unsafe because of concurrent effects on other threads. Therefore, it's plain crazy for ecpg to be calling setlocale() inside threaded code. It looks to me like what ecpg is doing is trying to defend itself against non-C LC_NUMERIC settings, which is laudable, but this implementation of that is totally unsafe. Don't know what's the best way out of this. The simplest thing would be to just remove that code and document that you'd better run ecpg in LC_NUMERIC locale, but it'd be nice if we could do better. regards, tom lane
