Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1140 closed defect (fixed)

loader/PointZ test failing on my windows 9.0/9.1

Reported by: robe Owned by: strk
Priority: medium Milestone: PostGIS 2.0.0
Component: postgis Version: master
Keywords: Cc:

Description

I haven't tried my 8.4 yet, I presume it's strk's last r7656 change?

It seems now my results are sorted differently from expected: The PointZ.select.expected has:

POINT(0 1 2 3)
POINT(9 -1 -20 -123)
POINT(9 -1 -2 -3)

But I'm getting

POINT(0 1 2 3)
POINT(9 -1 -2 -3)
POINT(9 -1 -20 -123)

I guess 0 sorts after space on windows.

What do you get strk:

e.g.

SELECT *
FROM (values ('01'),(' 03')) As f
ORDER By 1;

on my box gives:

 03
01

Attachments (2)

run_test_collate.diff (610 bytes) - added by ads 9 years ago.
add collate and template to createdb
regress_collate.patch (1.5 KB) - added by strk 9 years ago.
comprehensive patch for trunk, apply with patch -p1

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by robe

Owner: changed from pramsey to strk

update this also fails on my 8.4.8 windows vc++ build (against mingw compiled PostGIS)

comment:2 Changed 9 years ago by ads

I get the exact opposite order:

ads=# SELECT * FROM (values ('01'),(' 03')) AS f ORDER BY 1;
 column1 
---------
 01
  03
(2 Zeilen)

What is your Collate setting?
See: http://www.depesz.com/index.php/2011/06/30/why-is-depesz-between-de-luca-and-de-vil/

comment:3 Changed 9 years ago by strk

We're supposed to be running tests with the C locale, are we?

comment:4 Changed 9 years ago by ads

At least that's what run_test is doing:

# Set the locale to "C" so error messages match
# Save original locale to set back
ORIG_LC_ALL=$LC_ALL
ORIG_LANG=$LANG
export LC_ALL=C
export LANG=C

if [ -n "$USE_VERSION" ]; then
	if [ "$USE_VERSION" -gt 74 ]; then
		PGOPTIONS="${PGOPTIONS} -c lc_messages=C"
		export PGOPTIONS
	fi
fi

So why is this breaking the regression test?

comment:5 Changed 9 years ago by robe

I fiddled with the locale posix/c/us something or other -- doesn't change my sorting

comment:6 Changed 9 years ago by robe

I also tried on a different windows box running 8.4 - created a new db with local c

CREATE DATABASE test
  WITH OWNER = postgres
       ENCODING = 'UTF8'
       TABLESPACE = pg_default
       LC_COLLATE = 'C'
       LC_CTYPE = 'C'
       CONNECTION LIMIT = -1;


Ran my little test:

SELECT * FROM (values ('01'),(' 03')) AS f ORDER BY 1;

Output:

 column1
---------
  03
 01

comment:7 Changed 9 years ago by robe

hmm I wonder if I can't have UTF-8 database and C-locale. In the article you pointed to it says you can't do this:

ERROR: collation "c" for encoding "UTF8" does not exist

Indeed 9.1 has other issues

SELECT * FROM (values ('01'),(' 03')) AS f(a) ORDER BY 1 collate somethingorother;

gives
ERROR:  collations are not supported by type integer

Is illegal and it reads 1 as if it were an integer rather than a column.

I don't have any collations installed in my 9.1 so I created one which I would think would be illegal for UTF-8, but it let me do it:

CREATE COLLATION ucs_basic
  (LC_COLLATE='C', LC_CTYPE='C');
SELECT * FROM (values ('01'),(' 03')) AS f(a) ORDER BY a collate ucs_basic;

still didn't change the answer

Output:

 a
---------
  03
 01

comment:8 Changed 9 years ago by ads

Well, after further testing it looks like your sorting is right and my previous posting is misleading - and the test case seems to have a minor problem:
The only LC_* setting is LC_MESSAGES on top of the file.
LC_COLLATE is never set and therefore depends on whatever the database is using by default.

In addition, I can't create a database with LC_COLLATE='C' - unless I use template0 as template.

So the tests are only run with C locale, if you have set OS and database locale this way.

comment:9 Changed 9 years ago by ads

Hi, can you please try and test the attached patch with postgis-2.0.0SVN?
It works with PostgreSQL 9.0.x for me, but of course breaks my patch from #1135.

Changed 9 years ago by ads

Attachment: run_test_collate.diff added

add collate and template to createdb

comment:10 Changed 9 years ago by strk

I confirm it breaks the PointZ test:

--- loader/PointZ-w.select.expected     2011-07-20 21:51:54.000000000 +0200
+++ /tmp/pgis_reg_22295/test_3_out      2011-07-22 23:59:46.000000000 +0200
@@ -1,3 +1,3 @@
 POINT(0 1 2 3)
-POINT(9 -1 -20 -123)
 POINT(9 -1 -2 -3)
+POINT(9 -1 -20 -123)

Should we then apply and expect the new order ?

comment:11 Changed 9 years ago by ads

If everyone is fine with the new createdb options (please test in-depth) in run_test_collate.diff, I can provide a new patch for #1135.
Therefore it probably makes sense to revert #1135 first, run a number of tests on different platforms with the new patch, and then apply a new patch for #1135.
Opinions?

comment:12 Changed 9 years ago by strk

I think the simplest thing for everyone is to apply a single patch against current HEAD and see if it gives _no_ failures. When successful, we can do the same for 1.5 branch.

So I suggest you provide another all-inclusive patch containing both the run_test modification _and_ the .expected files modification.

Changed 9 years ago by strk

Attachment: regress_collate.patch added

comprehensive patch for trunk, apply with patch -p1

comment:13 Changed 9 years ago by strk

I attached the full patch for trunk. Successfully passes make check on my postgresql 8.4.8 on 64bit GNU/Linux

comment:14 Changed 9 years ago by ads

Well, I wanted to solve the collate problem first, before I apply for a new patch ;-)

comment:15 Changed 9 years ago by robe

Fix works for my 8.4, 9.0, and 9.1beta3 builds. Andrea I assume you are all set too? strk you want to go ahead and commit and close this out and the other out or you want me to?

Thanks, Regina

comment:16 Changed 9 years ago by robe

meant Andreas sorry for the typo.

comment:17 Changed 9 years ago by strk

@robe: feel free to go ahead if 'ads' confirms it works for him as well.

comment:18 Changed 9 years ago by strk

Resolution: fixed
Status: newclosed

Actually, I committed it as r7665 and we can tweak it further if needed.

comment:19 Changed 9 years ago by ads

Since it worked for me with PostgreSQL, I'm fine. Will backport the patch to Greenplum - but that's my part.

Note: See TracTickets for help on using tickets.