Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4941 closed defect (fixed)

ST_RemEdgeModFace and TopoGeo_addLineString cause invalid face mbr

Reported by: strk Owned by: strk
Priority: medium Milestone: PostGIS 3.2.0
Component: topology Version: 2.2.x
Keywords: Cc:

Description

Somehow the MBR column of the face table is getting ruined by ST_RemEdgeModFace:

=# SELECT * from validatetopology('invalid');
 error | id1 | id2 
-------+-----+-----
(0 rows)
=# select ST_RemEdgeModFace('invalid', 330);
DEBUG:  Drop of edge 330 merged faces 152 and 134
DEBUG:  Face1 box is GBOX((12.123711,65.127846),(12.20883,65.210075))
DEBUG:  Face2 box is GBOX((12.163308,65.163696),(12.163559,65.163818))
DEBUG:  Merged faces box is GBOX((12.123711,65.127846),(12.20883,65.210075))
 st_remedgemodface 
-------------------
               134
(1 row)
=# SELECT * from validatetopology('invalid');
DEBUG:  MBR expected:BOX(12.123710632324219 65.12784576416016,12.208829879760742 65.21007537841797) obtained:BOX(12.123711265448206 65.12785021295713,12.208829785296102 65.21007378815003)
       error        | id1 | id2
--------------------+-----+-----
 face has wrong mbr | 134 |
(1 row)

Change History (12)

comment:1 by strk, 3 years ago

I think the culprit is in the cb_updateFacesById callback defined in topology/postgis_topology.c as the function steps by text representation of the box, thus reducing its precision.

comment:2 by strk, 3 years ago

Work in progress fix: https://gitlab.com/postgis/postgis/-/merge_requests/35

It's not just in updateFacesById but also in the cb_insertFaces callback...

comment:3 by strk, 3 years ago

Version: main2.2.x

This bug was introduced when PostGIS Topology was implemented in C, thus 2.2.0 times

comment:4 by strk, 3 years ago

Here's a simplified testcase:

SELECT createtopology('invalid');

SELECT count(*) from (
  SELECT TopoGeo_addLineString('invalid',
'LINESTRING(
  12.123711265448206 65.12785021295713,
  12.123711265448206 65.21007378815003,
  12.208829785296102 65.21007378815003,
  12.208829785296102 65.12785021295713,
  12.123711265448206 65.12785021295713)
'
  )
) foo;

SELECT * from validatetopology('invalid');

SELECT count(*) from (
  SELECT TopoGeo_addLineString('invalid',
'LINESTRING(
  12.164016376530826 65.16373151830707,
  12.164026259700226 65.16368501062713,
  12.164003720888175 65.16368611107725,
  12.164016376530826 65.16373151830707
)')
) foo;
SELECT * from validatetopology('invalid');

comment:5 by strk, 3 years ago

Summary: ST_RemEdgeModFace causes invalid face mbrST_RemEdgeModFace and TopoGeo_addLineString cause invalid face mbr

comment:7 by strk, 3 years ago

It definitely seems to be related to float vs. double resolution of bbox, as I saw lwgeom_refresh_bbox() changing results

comment:8 by strk, 3 years ago

The problem seems to be with ST_BoundingDiagonal as well, disagreeing with ST_Envelope:

=# with inp as ( select 'POLYGON((12.123711265448206 65.12785021295713,12.123711265448206 65.21007378815003,12.208829785296102 65.12785021295713,12.123711265448206 65.12785021295713))'::geometry g ) select ST_XMin(g), ST_XMin(ST_Envelope(g)), ST_XMin(ST_BoundingDiagonal(g)) from inp;
      st_xmin       |      st_xmin       |      st_xmin       
--------------------+--------------------+--------------------
 12.123711265448206 | 12.123711265448206 | 12.123710632324219

comment:9 by Sandro Santilli <strk@…>, 3 years ago

In 4c84d49/git:

Return the full ("fit") face mbr from cb_getFacesById

Add test of face mbr invalidity on TopoGeo_addLineString

References #4941 in main branch (3.2.0dev)

comment:10 by Sandro Santilli <strk@…>, 3 years ago

In d7191135/git:

Return the full ("fit") face mbr from cb_getFacesById

Add test of face mbr invalidity on TopoGeo_addLineString

References #4941 in stable-3.1 branch (3.1.4dev)

comment:11 by Sandro Santilli <strk@…>, 3 years ago

Resolution: fixed
Status: newclosed

In 27ae21c5/git:

Return the full ("fit") face mbr from cb_getFacesById

Add test of face mbr invalidity on TopoGeo_addLineString

Closes #4941 in stable-3.0 branch (3.0.4dev)

comment:12 by strk, 3 years ago

Although 2.5 and 2.4 are also bogus, I won't backport the fix as the bug isn't a crasher and the slightly larger MBR isn't such a big deal (and I can't easily build for those branches either)

Note: See TracTickets for help on using tickets.