Re: B-Tree support function number 3 (strxfrm() optimization)

On Mon, Jan 19, 2015 at 7:47 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On MinGW-32, not that I know of:
> $ find . -name *.h | xgrep strxfrm_l
> ./lib/gcc/mingw32/4.8.1/include/c++/mingw32/bits/c++config.h:/* Define if strxfr
> m_l is available in <string.h>. */
> ./mingw32/lib/gcc/mingw32/4.8.1/include/c++/mingw32/bits/c++config.h:/* Define i
> f strxfrm_l is available in <string.h>. */
> strxfrm is defined in string.h though.

I'm not quite following. Doesn't that imply that strxfrm_l() at least
*could* be available? I guess it doesn't matter, though, because the
animal with the successful build that fails the locale regression test
(brolga) does not have locale_t support. Therefore, there is no new
strxfrm_l() caller.

My next guess is that the real problem is an assumption I've made.
That is, my assumption that strxfrm() always behaves equivalently to
strcpy() when the C locale happens to be in use may not be portable
(due to external factors). I guess we're inconsistent about making
sure that LC_COLLATE is set correctly in WIN32 and/or EXEC_BACKEND
builds, or something along those lines. The implementation in the past
got away with strcoll()/strxfrm() not having the C locale set, since
strcoll() was never called when the C locale was in use -- we just
called strcmp() instead.

Assuming that's correct, it might be easier just to entirely disable
the optimization on Windows, even with the C locale. It may not be
worth it to even bother just for C locale support of abbreviated keys.
I'm curious about what will happen there when the "_strxfrm_l()" fix
patch is applied.

> With your patch applied, the failure with MSVC disappeared, but there
> is still a warning showing up:
> (ClCompile target) ->
> src\backend\lib\hyperloglog.c(73): warning C4334: '<<' : result of
> 32-bit shift implicitly converted to 64 bits (was 64-bit shift
> intended?

That seems harmless. I suggest an explicit cast to "Size" here.

--
Peter Geoghegan