Re: Windows buildfarm animals are still not happy with abbreviated keys patch

On Thu, Jan 22, 2015 at 11:28 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> This seems to have broken more stuff. My working hypothesis is that
> the culprit is here:
>
> /*
> * There is no special handling of the C locale here, unlike with
> * varstr_cmp(). strxfrm() is used indifferently.
> */
>
> As far as I can see, that's just hoping that when the locale is C,
> strxfrm() is memcpy(). If it isn't, then you will potentially get
> different results when the abbreviated keys stuff is used vs. when it
> isn't, because when it isn't, we're going to memcmp(), and when it is,
> we're going to memcmp() the results of strxfrm().

As noted also on the thread Kevin started, I've now pushed a fix for
this and am eagerly awaiting new buildfarm returns. I wonder if this
might account for the ordering failures we were seeing on Windows
before. We thought it was a Windows-specific issue, but I bet the
real problem was here:

if (collid != DEFAULT_COLLATION_OID)
{
if (!OidIsValid(collid))
{
/*
* This typically means that the parser could
not resolve a
* conflict of implicit collations, so report
it that way.
*/
ereport(ERROR,

(errcode(ERRCODE_INDETERMINATE_COLLATION),
errmsg("could not determine
which collation to use for string comparison"),
errhint("Use the COLLATE
clause to set the collation explicitly.")));
}
#ifdef HAVE_LOCALE_T
tss->locale = pg_newlocale_from_collation(collid);
#endif
}

Before the abbreviated keys commit, this code only ran if we'd already
determined that lc_collate_is_c(collid) was false. But that commit
made this also run when that value was true. So if HAVE_LOCALE_T and
collid != DEFAULT_COLLATION_ID, then tss->locale was getting set. If
it got set to something that made strxfrm() behave like memcmp(), then
all was well. If not, then life was bad. Now you'd hope that would
work out, but maybe not.

Also, suppose HAVE_LOCALE_T was defined but collid ==
DEFAULT_COLLATION_ID. Then tss->locale didn't get initialized at all,
and bttext_abbrev_convert() just passed whatever stupid value it found
there to strxfrm_l().

Finally, suppose we *didn't* HAVE_LOCALE_T. Then, the
non-abbreviated comparator knew it should use memcmp(), but the
abbreviated comparator was happy to use strxfrm() even though that
would use the current locale, but the C locale that the user
requested.

Long story short: this was broken. It may be that when the dust
settles we can try re-enabling this on Windows. It might work now
that this issue is (hopefully) fixed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company