Fix crash splitting faces used by multiple TopoGeometry objects

Thanks rulus for spotting and analyzing the bug.
See #3407

git-svn-id: http://svn.osgeo.org/postgis/trunk@14522 b70326c6-7e19-0410-871a-916f4a2858ee

diff --git a/topology/postgis_topology.c b/topology/postgis_topology.c
index d7375dfd088eac381143de272e04e790c924a7f5..9a057f3aabe3676a6f67fbcbea603cd7cdbc72b4 100644 (file)
--- a/topology/postgis_topology.c
+++ b/topology/postgis_topology.c
@@ -2028,53 +2028,62 @@ cb_updateTopoGeomFaceSplit ( const LWT_BE_TOPOLOGY* topo,
   }
 
   ntopogeoms = SPI_processed;
-  for ( i=0; i<ntopogeoms; ++i )
+  if ( ntopogeoms )
   {
-    HeapTuple row = SPI_tuptable->vals[i];
-    TupleDesc tdesc = SPI_tuptable->tupdesc;
-    int negate;
-    int element_id;
-    int topogeo_id;
-    int layer_id;
-    int element_type;
+    resetStringInfo(sql);
+    appendStringInfo(sql, "INSERT INTO \"%s\".relation VALUES ", topo->name);
+    for ( i=0; i<ntopogeoms; ++i )
+    {
+      HeapTuple row = SPI_tuptable->vals[i];
+      TupleDesc tdesc = SPI_tuptable->tupdesc;
+      int negate;
+      int element_id;
+      int topogeo_id;
+      int layer_id;
+      int element_type;
+
+      if ( ! getNotNullInt32( row, tdesc, 1, &element_id ) ) {
+        cberror(topo->be_data,
+          "unexpected null element_id in \"%s\".relation",
+          topo->name);
+        return 0;
+      }
+      negate = ( element_id < 0 );
 
-    if ( ! getNotNullInt32( row, tdesc, 1, &element_id ) ) {
-                 cberror(topo->be_data,
-        "unexpected null element_id in \"%s\".relation",
-        topo->name);
-           return 0;
-    }
-    negate = ( element_id < 0 );
+      if ( ! getNotNullInt32( row, tdesc, 2, &topogeo_id ) ) {
+        cberror(topo->be_data,
+          "unexpected null topogeo_id in \"%s\".relation",
+          topo->name);
+        return 0;
+      }
 
-    if ( ! getNotNullInt32( row, tdesc, 2, &topogeo_id ) ) {
-                 cberror(topo->be_data,
-        "unexpected null topogeo_id in \"%s\".relation",
-        topo->name);
-           return 0;
-    }
+      if ( ! getNotNullInt32( row, tdesc, 3, &layer_id ) ) {
+        cberror(topo->be_data,
+          "unexpected null layer_id in \"%s\".relation",
+          topo->name);
+        return 0;
+      }
 
-    if ( ! getNotNullInt32( row, tdesc, 3, &layer_id ) ) {
-                 cberror(topo->be_data,
-        "unexpected null layer_id in \"%s\".relation",
-        topo->name);
-           return 0;
-    }
+      if ( ! getNotNullInt32( row, tdesc, 4, &element_type ) ) {
+        cberror(topo->be_data,
+          "unexpected null element_type in \"%s\".relation",
+          topo->name);
+        return 0;
+      }
 
-    if ( ! getNotNullInt32( row, tdesc, 4, &element_type ) ) {
-                 cberror(topo->be_data,
-        "unexpected null element_type in \"%s\".relation",
-        topo->name);
-           return 0;
-    }
+      if ( i ) appendStringInfoChar(sql, ',');
+      appendStringInfo(sql,
+        "(%d,%d,%" LWTFMT_ELEMID ",%d)",
+        topogeo_id, layer_id, negate ? -new_face1 : new_face1, element_type);
 
-    resetStringInfo(sql);
-    appendStringInfo(sql,
-      "INSERT INTO \"%s\".relation VALUES ("
-      "%d,%d,%" LWTFMT_ELEMID ",%d)", topo->name,
-      topogeo_id, layer_id, negate ? -new_face1 : new_face1, element_type);
+      if ( new_face2 != -1 ) {
+        appendStringInfo(sql,
+          ",(%d,%d,%" LWTFMT_ELEMID ",%d)",
+          topogeo_id, layer_id, negate ? -new_face2 : new_face2, element_type);
+      }
+    }
 
     POSTGIS_DEBUGF(1, "cb_updateTopoGeomFaceSplit query: %s", sql->data);
-
     spi_result = SPI_execute(sql->data, false, 0);
     MemoryContextSwitchTo( oldcontext ); /* switch back */
     if ( spi_result != SPI_OK_INSERT ) {
@@ -2084,31 +2093,11 @@ cb_updateTopoGeomFaceSplit ( const LWT_BE_TOPOLOGY* topo,
       return 0;
     }
     if ( SPI_processed ) topo->be_data->data_changed = true;
-    if ( new_face2 != -1 ) {
-      resetStringInfo(sql);
-      appendStringInfo(sql,
-        "INSERT INTO \"%s\".relation VALUES ("
-        "%d,%d,%" LWTFMT_ELEMID ",%d)", topo->name,
-        topogeo_id, layer_id, negate ? -new_face2 : new_face2, element_type);
-
-      POSTGIS_DEBUGF(1, "cb_updateTopoGeomFaceSplit query: %s", sql->data);
-
-      spi_result = SPI_execute(sql->data, false, 0);
-      MemoryContextSwitchTo( oldcontext ); /* switch back */
-      if ( spi_result != SPI_OK_INSERT ) {
-        cberror(topo->be_data, "unexpected return (%d) from query execution: %s",
-                spi_result, sql->data);
-        pfree(sqldata.data);
-        return 0;
-      }
-      if ( SPI_processed ) topo->be_data->data_changed = true;
-    }
   }
 
-  /* TODO: release string info */
-
   POSTGIS_DEBUGF(1, "cb_updateTopoGeomFaceSplit: updated %d topogeoms", ntopogeoms);
 
+  pfree(sqldata.data);
   return 1;
 }
 

diff --git a/topology/test/regress/st_addedgemodface.sql b/topology/test/regress/st_addedgemodface.sql
index 7c35188477946d50b04913975a9087ab0700b804..31caa4481331e6d11583b647cffb79fc6bafa10d 100644 (file)
--- a/topology/test/regress/st_addedgemodface.sql
+++ b/topology/test/regress/st_addedgemodface.sql
@@ -461,6 +461,20 @@ SELECT 'T28', 'E'||edge_id, next_left_edge, next_right_edge,
     UNION VALUES (4),(5) )
   ORDER BY edge_id;
 
+--
+-- Split a face referenced by multiple TopoGeometries
+--
+-- See https://trac.osgeo.org/postgis/ticket/3407
+--
+INSERT INTO city_data.fp VALUES ('F17',
+  topology.CreateTopoGeom('city_data', 3, 1, '{{17,3}}'));
+INSERT INTO newedge SELECT 29 as id, topology.st_addedgemodface('city_data',
+  14, 16, 'LINESTRING(21 14, 9 22)');
+SELECT 'T29', 'E'||edge_id, next_left_edge, next_right_edge,
+  left_face, right_face FROM
+  city_data.edge WHERE edge_id IN (21, 33, 19, 34,
+  ( SELECT edge_id FROM newedge WHERE id = 29 ) )
+  ORDER BY edge_id;
 
 ---------------------------------------------------------------------
 -- Check new relations and faces status
@@ -489,4 +503,3 @@ SELECT 'F'||face_id, st_astext(mbr) FROM city_data.face ORDER BY face_id;
 
 DROP TABLE newedge;
 SELECT topology.DropTopology('city_data');
-

diff --git a/topology/test/regress/st_addedgemodface_expected b/topology/test/regress/st_addedgemodface_expected
index 69d18c4481b9ba72f949fa69f1a1b02fcbec8de4..31b944c134ad45e7a544448e7696483e6ed1a233 100644 (file)
--- a/topology/test/regress/st_addedgemodface_expected
+++ b/topology/test/regress/st_addedgemodface_expected
@@ -162,7 +162,13 @@ T28|E44|-43|43|0|34
 T28|E52|-53|4|32|24
 T28|E53|-5|52|34|32
 T28|E54|5|-54|34|33
-F3,F4|{3:3,3:4,3:10,3:16,3:17}
+T29|E19|33|27|17|10
+T29|E21|6|34|0|35
+T29|E33|-55|-6|17|3
+T29|E34|55|9|35|16
+T29|E55|-21|19|35|17
+F17|{3:17,3:35}
+F3,F4|{3:3,3:4,3:10,3:16,3:17,3:35}
 F5,N4|{1:4,3:5,3:11}
 F0|
 F1|POLYGON((3 30,3 38,16 38,16 30,3 30))
@@ -199,4 +205,5 @@ F31|POLYGON((19 31,19 38,26 38,26 31,19 31))
 F32|POLYGON((36 33,36 38,57 38,57 33,36 33))
 F33|POLYGON((38 40,38 43,41 43,41 40,38 40))
 F34|POLYGON((35 25,35 45,63 45,63 25,35 25))
+F35|POLYGON((9 14,9 22,21 22,21 14,9 14))
 Topology 'city_data' dropped

diff --git a/topology/test/regress/st_addedgenewfaces.sql b/topology/test/regress/st_addedgenewfaces.sql
index b3b296f1d3f623f62b7a9bbd755b27e41e014fc1..e86f7f9f2ea29c5a24298ec669d56ddbe4f913b9 100644 (file)
--- a/topology/test/regress/st_addedgenewfaces.sql
+++ b/topology/test/regress/st_addedgenewfaces.sql
@@ -461,6 +461,21 @@ SELECT 'T28', 'E'||edge_id, next_left_edge, next_right_edge,
     UNION VALUES (4),(5) )
   ORDER BY edge_id;
 
+--
+-- Split a face referenced by multiple TopoGeometries
+--
+-- See https://trac.osgeo.org/postgis/ticket/3407
+--
+INSERT INTO city_data.fp VALUES ('F25',
+  topology.CreateTopoGeom('city_data', 3, 1, '{{25,3}}'));
+INSERT INTO newedge SELECT 29 as id, topology.st_addedgemodface('city_data',
+  14, 16, 'LINESTRING(21 14, 9 22)');
+SELECT 'T29', 'E'||edge_id, next_left_edge, next_right_edge,
+  left_face, right_face FROM
+  city_data.edge WHERE edge_id IN (21, 33, 19, 34,
+  ( SELECT edge_id FROM newedge WHERE id = 29 ) )
+  ORDER BY edge_id;
+
 ---------------------------------------------------------------------
 -- Check new relations and faces status
 ---------------------------------------------------------------------

diff --git a/topology/test/regress/st_addedgenewfaces_expected b/topology/test/regress/st_addedgenewfaces_expected
index 8fa933c2716ab92ae7b9c32a0a6cc56b7ff857d6..f98b2290d3357d5c9874c37a7dad7532d75b8bcc 100644 (file)
--- a/topology/test/regress/st_addedgenewfaces_expected
+++ b/topology/test/regress/st_addedgenewfaces_expected
@@ -162,7 +162,13 @@ T28|E44|-43|43|0|50
 T28|E52|-53|4|47|45
 T28|E53|-5|52|50|47
 T28|E54|5|-54|50|49
-F3,F4|{3:10,3:11,3:22,3:24,3:25}
+T29|E19|33|27|25|11
+T29|E21|6|34|0|51
+T29|E33|-55|-6|25|22
+T29|E34|55|9|51|24
+T29|E55|-21|19|51|25
+F25|{3:25,3:51}
+F3,F4|{3:10,3:11,3:22,3:24,3:25,3:51}
 F5,N4|{1:4,3:12,3:13}
 F0|
 F1|POLYGON((3 30,3 38,16 38,16 30,3 30))
@@ -199,4 +205,5 @@ F45|POLYGON((36 28,36 38,57 38,57 28,36 28))
 F47|POLYGON((36 33,36 38,57 38,57 33,36 33))
 F49|POLYGON((38 40,38 43,41 43,41 40,38 40))
 F50|POLYGON((35 25,35 45,63 45,63 25,35 25))
+F51|POLYGON((9 14,9 22,21 22,21 14,9 14))
 Topology 'city_data' dropped