Opened 12 years ago

Closed 11 years ago

#1998 closed defect (fixed)

Crash of topogeo_AddPolygon() with "ERROR: query string argument of EXECUTE is null"

Reported by: wimned Owned by: strk
Priority: blocker Milestone: PostGIS 2.0.2
Component: topology Version: master
Keywords: topogeo_AddPolygon, history Cc:

Description

Complete message:

ERROR: query string argument of EXECUTE is null CONTEXT: PL/pgSQL function "st_modedgesplit" line 106 at EXECUTE statement PL/pgSQL function "topogeo_addpoint" line 65 at assignment PL/pgSQL function "topogeo_addlinestring" line 92 at assignment SQL statement "SELECT array_cat(edges, array_agg(x)) FROM ( select topology.TopoGeo_addLinestring(atopology, rec.geom, tol) as x ) as foo"

Version:

POSTGIS="2.1.0SVN r10195" GEOS="3.3.4-CAPI-1.7.3" PROJ="Rel. 4.7.1, 23 September 2009" LIBXML="2.7.8" TOPOLOGY

After pol1 is added, node 2, the begin and end of the loop of pol1 is removed through ST_ModEdgeHeal(). The code breaks when pol2 is added after this. Rearranging the geometry of pol1, so the common point of pol0 and pol1 is the begin/endpoint of the loop of pol1, prevents the crash.

The code:

CREATE OR REPLACE FUNCTION testerror1()
 returns void as
$$
    declare pol0 geometry;
    declare pol1 geometry;
    declare pol2 geometry;
    declare node_rec RECORD;
begin
    raise notice 'version: %', postgis_full_version();
    
    perform CreateTopology('wimpy', 4326, 0.0000001);

    CREATE VIEW face_geom AS
        select i.face_id, ST_GetFaceGeometry('wimpy', i.face_id) as geom
            from wimpy.face as i
                where face_id != 0;

    pol0 = ST_GeometryFromText(
        'POLYGON((76.5282669067383 9.43292331695557,76.5287322998047 9.43261814117432,76.5285415649414 9.43255615234375,76.5282669067383 9.43292331695557))', 4326);

    pol1 = ST_GeometryFromText(
        'POLYGON((76.5267181396484 9.4406852722168,76.5282669067383 9.43292331695557, 76.5264434814453 9.43406867980957, 76.5267181396484 9.4406852722168))',4326);

    pol2 = ST_GeometryFromText(
        'POLYGON((76.524 9.437,
                76.5267181396484 9.4406852722168,76.5264434814453 9.43406867980957,76.524 9.437))', 4326);


    perform topogeo_AddPolygon('wimpy', pol0);
    perform topogeo_AddPolygon('wimpy', pol1);
    perform cleanse_nodes();
    perform topogeo_AddPolygon('wimpy', pol2);
END
$$
LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION cleanse_nodes()
  returns void AS
$$
  DECLARE node_rec RECORD;
BEGIN
    for node_rec in select node_id from node
    loop
        perform cleanse_node(node_rec.node_id);
    end loop;
END
$$
LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION cleanse_node(passed_node_id integer)
  returns void AS
$$
  DECLARE edge_ids integer[];
BEGIN
    --raise notice 'check node %', passed_node_id;
    select array_agg(abs(t.edge)) into edge_ids
        from GetNodeEdges('wimpy', passed_node_id) as t(seq,edge);

    if array_length(edge_ids,1) = 2 and edge_ids[1] != edge_ids[2]
    then
        raise notice 'remove node %', passed_node_id;

        perform ST_ModEdgeHeal('wimpy', edge_ids[1], edge_ids[2]);
    end if;
END
$$
LANGUAGE plpgsql;

Attachments (1)

3pols.jpg (17.3 KB ) - added by wimned 12 years ago.
the polygons

Download all attachments as: .zip

Change History (17)

by wimned, 12 years ago

Attachment: 3pols.jpg added

the polygons

comment:1 by strk, 12 years ago

Component: postgistopology
Milestone: PostGIS 2.1.0PostGIS 2.0.2
Owner: changed from pramsey to strk

Note that the test script doesn't work on PostgreSQL 9.1.5. It's a search_path issue, your cleanse_node function references a "node" table but maybe should reference a "wimpy.node" one

in reply to:  1 comment:3 by wimned, 12 years ago

Replying to strk:

Note that the test script doesn't work on PostgreSQL 9.1.5. It's a search_path issue, your cleanse_node function references a "node" table but maybe should reference a "wimpy.node" one

I call "export PGOPTIONS="-c SEARCH_PATH=wimpy,topology,public"" in the calling bash script.

comment:4 by strk, 12 years ago

The topology is made invalid by cleanse_node:

=# select * from validatetopology('wimpy');
               error                | id1 | id2 
------------------------------------+-----+-----
 edge crosses edge                  |   1 |   2
 edge start node geometry mis-match |   2 |   1
 edge end node geometry mis-match   |   2 |   1
(3 rows)

Before the cleansing you have 3 edges and 2 nodes in a valid topology.

comment:5 by strk, 12 years ago

So basically the bug is in ST_ModEdgeHeal, which lets you break the topology by picking the wrong node to heal at. Need a smaller testcase next, to put with the rest of tests in the suite.

comment:6 by strk, 12 years ago

The problem is with the assumption that ST_LineMerge will do the right thing, which of course can't be true. So the node is choosed correctly, but the line merging is done in the wrong way.

comment:7 by wimned, 12 years ago

smaller testcase:

CREATE OR REPLACE FUNCTION testerror1()
 returns void as
$$
    declare pol0 geometry;
    declare pol1 geometry;
begin
    raise notice 'version: %', postgis_full_version();
    
    perform CreateTopology('wimpy', 4326, 0.0000001);

    pol0 = ST_GeometryFromText(
        'POLYGON((1 1,1 2,2 2,2 1,1 1))', 4326);

    pol1 = ST_GeometryFromText(
        'POLYGON((0 0,0 1,1 1,1 0,0 0))', 4326);

    perform topogeo_AddPolygon('wimpy', pol0);
    perform topogeo_AddPolygon('wimpy', pol1);
    perform cleanse_nodes();
END
$$
LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION cleanse_nodes()
  returns void AS
$$
  DECLARE node_rec RECORD;
BEGIN
    for node_rec in select node_id from wimpy.node
    loop
        perform cleanse_node(node_rec.node_id);
    end loop;
END
$$
LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION cleanse_node(passed_node_id integer)
  returns void AS
$$
  DECLARE edge_ids integer[];
BEGIN
    --raise notice 'check node %', passed_node_id;
    select array_agg(abs(t.edge)) into edge_ids
        from GetNodeEdges('wimpy', passed_node_id) as t(seq,edge);

    if array_length(edge_ids,1) = 2 and edge_ids[1] != edge_ids[2]
    then
        raise notice 'remove node %', passed_node_id;

        perform ST_ModEdgeHeal('wimpy', edge_ids[1], edge_ids[2]);
    end if;
END
$$
LANGUAGE plpgsql;

from validate_topology:

               error                | id1 | id2 
------------------------------------+-----+-----
 edge crosses edge                  |   1 |   2
 edge start node geometry mis-match |   2 |   1
 edge end node geometry mis-match   |   2 |   1
(3 rows)

edges:

 edge_id |            st_astext            
---------+---------------------------------
       2 | LINESTRING(0 0,1 0,1 1,0 1,0 0)
       1 | LINESTRING(1 1,1 2,2 2,2 1,1 1)
(2 rows)


nodes:

 node_id | st_astext  
---------+------------
       1 | POINT(1 1)
(1 row)

looks a bit similar to ticket #1955

comment:8 by wimned, 12 years ago

Created a workaround: After call of ST_ModEdgeHeal() the first point of geometry of the new edge is checked against the geometry of the start node. When not, points are shifted in the edge geometry until the first point matches the start node. The edge is removed using ST_RemEdgeModFace(). The fixed geometry is added using topogeo_AddLineString()

The code (probably could be made less clumsy…):

CREATE OR REPLACE FUNCTION cleanse_node(passed_node_id integer)
  returns void AS
$$
  DECLARE edge_ids integer[];
  DECLARE start_node_id integer;
  DECLARE node_geom geometry;
  DECLARE edge_geom geometry;
  DECLARE edge_sz integer;
BEGIN
    --raise notice 'check node %', passed_node_id;
    select array_agg(abs(t.edge)) into edge_ids
        from GetNodeEdges('wimpy', passed_node_id) as t(seq,edge);

    if array_length(edge_ids,1) = 2 and edge_ids[1] != edge_ids[2]
    then
        raise notice 'remove node %', passed_node_id;

        perform ST_ModEdgeHeal('wimpy', edge_ids[1], edge_ids[2]);

        select e.start_node into start_node_id from
            wimpy.edge_data as e where e.edge_id = edge_ids[1]; 
        select e.geom into edge_geom from
            wimpy.edge_data as e where e.edge_id = edge_ids[1]; 
        select n.geom into node_geom from
            wimpy.node as n where n.node_id = start_node_id;

        if not ST_Equals(node_geom, ST_PointN(edge_geom,1))
        then
            raise notice 'edge % needs fixing at %', edge_ids[1], ST_ASText(node_geom);
            edge_geom = ST_RemovePoint(edge_geom, 0);
            edge_sz = ST_NPoints(edge_geom);
            for i in 1..edge_sz
            loop
                exit when ST_Equals(node_geom, ST_PointN(edge_geom,1)); 
                edge_geom = ST_AddPoint(edge_geom, ST_PointN(edge_geom,1), -1);
                edge_geom = ST_RemovePoint(edge_geom, 0);
            end loop;
            edge_geom = ST_AddPoint(edge_geom, node_geom, -1);
            perform ST_RemEdgeModFace('wimpy', edge_ids[1]);
            perform topogeo_AddLineString('wimpy', edge_geom);
        end if;
    end if;
END
$$

comment:9 by wimned, 12 years ago

Question: is this ticket going to be assigned? I'm not in a hurry though, my workaround just works fine.

comment:10 by strk, 12 years ago

Status: newassigned

Yep, consider it assigned to me. Not sure when I'll find the time to fix it though. Surely needs to be fixed before 2.0.2 is out.

comment:11 by robe, 11 years ago

strk - is this still an issue — are you okay with pushing this or are you going to get your butt in gear :)

comment:12 by strk, 11 years ago

I'll try to get my butt started

comment:13 by strk, 11 years ago

Priority: mediumblocker

comment:14 by strk, 11 years ago

Further simplification:

SELECT CreateTopology('t1998');
SELECT ST_AddIsoNode('t1998', 0, 'POINT(1 1)');
SELECT ST_AddIsoNode('t1998', 0, 'POINT(0 0)');
SELECT ST_AddEdgeModFace('t1998', 1, 1, 'LINESTRING(1 1,1 2,2 2,2 1,1 1)');
SELECT ST_AddEdgeModFace('t1998', 2, 1, 'LINESTRING(0 0,0 1,1 1)');
SELECT ST_AddEdgeModFace('t1998', 1, 2, 'LINESTRING(1 1,1 0,0 0)');
SELECT 'V1', ValidateTopology('t1998'); -- ok
SELECT ST_ModEdgeHeal('t1998', 2, 3);
SELECT 'V2', ValidateTopology('t1998'); -- mess

comment:15 by strk, 11 years ago

Ok so the curious effect of this case is that you start from the upper node, check which edges are sharing that node and request healing them. To the possible user surprise the edges _are_ healed, but the removed node is on the other side. I think the invariant we want to keep here is the direction of the first edge passed to the ModEdge function, but we of course have to give up the invariant that the start node of the first edge will remain the start node of the resulting "merged" edge (because in this case the first edge passed to the function will end up being the _second_ portion of the merged edge).

comment:16 by strk, 11 years ago

Keywords: history added
Resolution: fixed
Status: assignedclosed

r10733 in 2.0 branch, r10734 in trunk.

Note: See TracTickets for help on using tickets.