Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#1023 closed defect (fixed)

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

Reported by: robe Owned by: pramsey
Priority: critical Milestone: PostGIS 2.0.0
Component: postgis Version: trunk
Keywords: Cc:

Description

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.

Attachments (3)

pgis84_topology_regress.zip (1.5 KB) - added by robe 6 years ago.
pgis90_topology_regress.zip (1.8 KB) - added by robe 6 years ago.
pgis91b2_topology_regress.zip (2.2 KB) - added by robe 6 years ago.

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by robe

Attachment: pgis84_topology_regress.zip added

Changed 6 years ago by robe

Attachment: pgis90_topology_regress.zip added

Changed 6 years ago by robe

comment:1 Changed 6 years ago by 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.

comment:2 Changed 6 years ago by 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...

comment:3 Changed 6 years ago by 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)

comment:4 Changed 6 years ago by 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

comment:5 Changed 6 years ago by 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...

comment:6 Changed 6 years ago by 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...

comment:7 Changed 6 years ago by strk

Component: topologypostgis
Owner: changed from strk to pramsey
Summary: topology regress failures related to GSERIALIZED: copytopology andbtree equality broken for geometries w/out a cached bounding box (topology regress failures related to GSERIALIZED)

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.

comment:8 Changed 6 years ago by 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)

comment:9 Changed 6 years ago by 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...

comment:10 Changed 6 years ago by strk

Summary: btree equality broken for geometries w/out a cached bounding box (topology regress failures related to GSERIALIZED)pglwgeom_

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.

comment:11 Changed 6 years ago by 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
}

comment:12 Changed 6 years ago by strk

Summary: pglwgeom_pglwgeom_getbox2d_p fails to compute a bounding box if a cache is missing (GSERIALIZED)

comment:13 Changed 6 years ago by strk

Priority: highcritical

comment:14 Changed 6 years ago by strk

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

comment:15 Changed 6 years ago by strk

Resolution: fixed
Status: newclosed

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)

comment:16 Changed 6 years ago by robe

yap fixes it.

Note: See TracTickets for help on using tickets.