pglwgeom_getbox2d_p fails to compute a bounding box if a cache is missing (GSERIALIZED)

[robe]

strk, On 8.4.4, 9.0.2, 9.1beta2 windows:

copytopology gives regress failure — might just be a rounding issue. The 9.0.2 one fails in more spots.

On 9.1beta2 both

copytopology and createtopogeom tests fail.

The createtopogeom seems harmless difference in line number error so you might just want to do your magic and strip the error numbers.

I'm attaching the regress failure logs.

Once this gets settled I think we are clean as far as our basic regress tests go. Then we'll try to release another windows build for others to stress test and will do some more involved stress testing ourselves.

[strk]

For CopyTopology regress test the offending query for the 8.4 case is:

SELECT * FROM "CITY_data_UP_down".node EXCEPT
SELECT * FROM "city_data".node;

It actually fails for me too on 8.4.8 (succeeds in 8.4.3). It is probably related to btree index for geometry. Did its semantic change ? It used to be bounding box equality.

Interesting enough it only fails for points, not for edges (which also have geometries involved).

My failure case is on 32bit GNU/Linux and returns all of the 22 nodes from the query. In your case a single row is returned.

[strk]

The idea of the test is to make sure that all we had in one table is also present in the other table. I've to say that a bounding-box only btree check there isn't that strong so could change the test to use hexwkb output for geometries instead.

But then I wonder if btree should really be bounding-box based only…

[strk]

Example effects of a bounding-box based btree:

=> select 'LINESTRING(0 0, 10 10)'::geometry except select 'LINESTRING(10 10, 10 0, 0 0)'::geometry;

 geometry 
----------
(0 rows)

[strk]

Now this seems interesting:

=# select version();
-[ RECORD 1 ]------------------------------------------------------------------------------------------------------
version | PostgreSQL 8.4.8 on i486-pc-linux-gnu, compiled by GCC gcc-4.4.real (Ubuntu 4.4.3-4ubuntu5) 4.4.3, 32-bit

=# select a.geom as a, b.geom as b, a=b as equal from city_data.node a, city_data.node b where a.node_id = b.node_id and a.node_id = 1;
-[ RECORD 1 ]-------------------------------------
a     | 010100000000000000000020400000000000003E40
b     | 010100000000000000000020400000000000003E40
equal | f

[strk]

Adding some debugging to the postgis code I get these crappy values:

NOTICE:  box1: 3.52137e-148 -1.63709e-50, 7.32492e-307 3.49705e-148
NOTICE:  box2: 3.46592e-148 2.1382e-148, -0.180213 2.1382e-148

Note that this is the _same_ geometry…

[strk]

pglwgeom_getbox2d_p is the code used to extract the bounding box from the serialized form in postgis/lwgeom_btree.c. Probably that's not the function that should be used…

[strk]

Found the problem: pglwgeom_getbox2d_p is expected to _compute_ the bounding box when no cached. In the non-gserialized flavor it does so (getbox2d_p in lwgeom_api.c) but in the gserialized flavor it doesn't (gserialized_get_box_p in g_serialized.c)

The pglwgeom_getbox2d_p function is NOT documented to require computation, but I do know it (eh…).

So:

1. Document behavior
2. Fix it for GSERIALIZED

I'd like to hear from Paul about this first though, in case he has other clients relying on gserialized_get_gbox_p NOT computing a missing box.

What I could do meanwhile is providing a regression test for this bug, which is _unrelated_ to topologies.

[strk]

Even simpler case:

=# select 'POINT(10 4)'::geometry = 'POINT(10 4)'::geometry;
NOTICE:  box1: 2.6459e-148 6.96305e-309, 2.64864e-306 9.86915e-306
NOTICE:  box2: -0.180213 0, -0.180289 6.95336e-309
 ?column?
----------
 f
(1 row)

[strk]

Note that LWGEOM_gist_compress also expects a false return from pglwgeom_getbox2d_p meaning EMPTY geom, so it's very likely that gist index of point is broken with GSERIALIED on…

[strk]

So, turns out the actually used functions _are_ documented, only they are not compatible:

/*
 * this function writes to 'box' and returns 0 if serialized_form
 * does not have a bounding box (empty geom)
 */
extern int getbox2d_p(uchar *serialized_form, BOX2DFLOAT4 *box);

/**
* Pull a #GBOX from the header of a #GSERIALIZED, if one is available. If
* it is not, return LW_FAILURE.
*/
extern int gserialized_get_gbox_p(const GSERIALIZED *g, GBOX *gbox);

int pglwgeom_getbox2d_p(const PG_LWGEOM *geom, BOX2DFLOAT4 *box)
{       
#ifdef GSERIALIZED_ON
        return gserialized_get_gbox_p(geom, box);
#else           
        return getbox2d_p(SERIALIZED_FORM(geom), box);
#endif
}

So basically pglwgeom_getbox2d_p is using one or the other function but while one returns a bounding box if NON-empty the other only returns it if CACHED.

[strk]

And I'm afraid computing box2d from a gserialized isn't implemented:

int pglwgeom_compute_serialized_box3d_p(const PG_LWGEOM *geom, BOX3D *box3d)
{
#ifdef GSERIALIZED_ON
        lwerror("pglwgeom_compute_serialized_box3d_p called!");
        return 0;
#else
        return compute_serialized_box3d_p(SERIALIZED_FORM(geom), box3d);
#endif
}

[strk]

gserialized_datum_get_gidx_p in gserialized_gist_nd.c is the closest we have to what we need.

[strk]

Alright, I've fixed this by adding the *compute_if_missing* logic into the pglwgeom_getbox2d_p function, so that the underlying function retain their documented behavior. r7582 documents pglwgeom_getbox2d_p in header, r7583 implements it correcly for G_SERIALIZED on.

Robe: can you confirm it fixes the topology tests your side ? (it does fix mine againts 8.4.8)

[robe]

yap fixes it.