postgrespro / pgsphere

PgSphere provides spherical data types, functions, operators, and indexing for PostgreSQL.
https://pgsphere.org
BSD 3-Clause "New" or "Revised" License
16 stars 15 forks source link

sellipse/scircle overlaps regression tests #61

Closed df7cb closed 10 months ago

df7cb commented 10 months ago

Hi,

on 32-bit i386 Debian Linux I see a regression failure:

**** regression.diffs ****
diff -U3 /<<PKGBUILDDIR>>/expected/overlaps.out /<<PKGBUILDDIR>>/results/overlaps.out
--- /<<PKGBUILDDIR>>/expected/overlaps.out  2023-08-31 05:45:01.000000000 +0000
+++ /<<PKGBUILDDIR>>/results/overlaps.out   2023-09-05 10:13:42.121934808 +0000
@@ -963,7 +963,7 @@
 select 'sbox && sellipse', 'f' as expected, sbox'((-10d , -10d), (10d , 10d))' && sellipse'<{ 80d , 10d }, (0d, 90d) , 90d>' as actual;
      ?column?     | expected | actual 
 ------------------+----------+--------
- sbox && sellipse | f        | f
+ sbox && sellipse | f        | t
 (1 row)

 -- the ellipse lies completely in the box and they have an intersection at the boundary points
### End 15 installcheck (FAILED with exit code 1) ###

Also, this bit from 56ff36668bb4d looks fishy:

diff --git a/expected/overlaps.out b/expected/overlaps.out
index 5a7625f..5b1f885 100644
--- a/expected/overlaps.out
+++ b/expected/overlaps.out
@@ -835,14 +835,14 @@ select 'sbox && scircle', 'f' as expected, sbox'((0d , 0d), (20d , 20d))' && sci
 select 'sbox && scircle', 'f' as expected, sbox'((0d , 0d), (0d , 0d))' && scircle'<(10d , 0d) , 10d>' as actual;
     ?column?     | expected | actual
 -----------------+----------+--------
- sbox && scircle | f        | f
+ sbox && scircle | f        | t
 (1 row)

 -- the box degenerated into the point lies in the interior of circle
 select 'sbox && scircle', 'f' as expected, sbox'((0d , 0d), (0d , 0d))' && scircle'<(0d , 0d) , 10d>' as actual;
     ?column?     | expected | actual
 -----------------+----------+--------
- sbox && scircle | f        | f
+ sbox && scircle | f        | t
 (1 row)

 -- the box and circle are degenerated into the point and coincide

Why was the "actual" value updated? It's not failing, but if there is an "expected" column, they should match.

vitcpp commented 10 months ago

@df7cb The overlaps.sql test purpose is to highlight the differences between DE9-IM semantics of overlaps operation and the behaviour of pgsphere's overlap operation. The expected column contains the value as described in DE9-IM as it should be if pgsphere follows DE9-IM. The actual column shows the current result of the operation.

The differences in 'actual' column should be investigated and fixed. I'm not sure why it happens now. Definitely, we have to fix such difference in behaviour on 64 and 32 bit platforms. Thank you very much for reporting.

The patch 56ff366 fixes the read of unitialized memory that was revealed by overlaps.sql test when compiling with different optimization gcc settings (O0, O2). It is pretty simple fix. I don't exclude that same problem may appear in other places.

We have some plans to add more platforms for testing, including 32 bit platforms as well.

vitcpp commented 10 months ago

@df7cb I've run pgsphere tests on FreeBSD 13.2-RELEASE GENERIC i386 (without healpix). All tests are passed ok. Did you use 1.3.0 version (latest)? Which platform did you use to run the tests?

vitcpp commented 10 months ago

I reproduced the test fail on 32 bit Debian Linux on my virtual box.

vitcpp commented 10 months ago

The problem originates in FPge function. It may produce different results when using MMX on 64 bit and i32 FPU. There is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=323 bug. It can be fixed by gmake CFLAGS=-ffloat-store when compiling pgsphere but it may affect the performance. It seems that comparison functions FPeq/ne/lt/le/gt/ge should be slightly changed in case of 32 bit platforms. I'm working on it.

df7cb commented 10 months ago

Hmm, -ffloat-store doesn't help here (I would have tried -fexcess-precision=standard, but that doesn't help either). It's great that you found something!

vitcpp commented 10 months ago

@df7cb -ffloat-store worked on my side on Debian 32 bit under Virtual Box. Please, make sure that you recompiled all files with this option. Could you please try my patch #62 ? It works on my side.

df7cb commented 10 months ago

Your patch fixes the problem :heavy_check_mark: