Opened 5 years ago

Closed 4 years ago

#4675 closed enhancement (fixed)

Implement topology.GetRingEdges in C

Reported by: strk Owned by: strk
Priority: medium Milestone: PostGIS 3.1.0
Component: topology Version: master
Keywords: Cc:

Description

At the moment we have two implementations of GetRingEdges:

  1. A PL/Pgsql version that is hit by running topology.GetRingEdges
  2. A C version cb_getRingEdges that is used internally as a callback for liblwgeom-topo

It would make sense to use a single code path, the C one probably being the best choice.

Change History (7)

comment:1 by Sandro Santilli <strk@…>, 5 years ago

Resolution: fixed
Status: newclosed

In ecae381/git:

Implement GetRingEdges in C

Closes #4675

comment:2 by strk, 5 years ago

Milestone: PostGIS 3.1.0
Type: defectenhancement

comment:3 by strk, 5 years ago

For the record, the C version is 21% faster.

Tested with following input:

SELECT DropTopology('city_data');

\i topology/test/load_topology.sql

select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
10, 10)) from city_data.face where face_id >0;
select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
11, 10)) from city_data.face where face_id >0;
select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
12, 10)) from city_data.face where face_id >0;
select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
13, 10)) from city_data.face where face_id >0;
select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
16, 10)) from city_data.face where face_id >0;

select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
10, 11)) from city_data.face where face_id >0;
select topology.TopoGeo_AddPolygon('city_data',ST_Translate(ST_GetFaceGeometry('city_data',face_id),
10, -20)) from city_data.face where face_id >0;

And following query:

explain (analyze, buffers) 
select count(*) from ( 
 select edge_id, (getRingEdges('city_data', edge_id)).* 
 from city_data.edge 
) foo;

Results in following plans:

PLPGSQL:

Aggregate  (cost=5040787.79..5040787.79 rows=1 width=8) (actual time=2646.457..2646.457 rows=1 loops=1)
  Buffers: shared hit=804888
  ->  Result  (cost=0.29..4921412.79 rows=9550000 width=12) (actual time=54.797..2634.831 rows=258613 loops=1)
        Buffers: shared hit=804888
        ->  ProjectSet  (cost=0.29..50912.79 rows=9550000 width=32) (actual time=54.792..2609.541 rows=258613 loops=1)
              Buffers: shared hit=804888
              ->  Index Only Scan using edge_data_pkey on edge_data  (cost=0.29..727.53 rows=9550 width=4) (actual time=0.024..3.815 rows=9550 loops=1)
                    Heap Fetches: 0
                    Buffers: shared hit=146
Planning Time: 0.319 ms
JIT:
  Functions: 6
  Options: Inlining true, Optimization true, Expressions true, Deforming true
  Timing: Generation 0.362 ms, Inlining 31.182 ms, Optimization 11.497 ms, Emission 11.008 ms, Total 54.049 ms
Execution Time: 2674.792 ms

C:

Aggregate  (cost=311174.16..311174.17 rows=1 width=8) (actual time=2072.915..2072.915 rows=1 loops=1)
  Buffers: shared hit=824179
  ->  Result  (cost=0.29..191799.16 rows=9550000 width=12) (actual time=19.584..2059.942 rows=258613 loops=1)
        Buffers: shared hit=824179
        ->  ProjectSet  (cost=0.29..48549.16 rows=9550000 width=32) (actual time=19.576..2034.256 rows=258613 loops=1)
              Buffers: shared hit=824179
              ->  Index Only Scan using edge_data_pkey on edge_data  (cost=0.29..727.53 rows=9550 width=4) (actual time=0.034..3.536 rows=9550 loops=1)
                    Heap Fetches: 0
                    Buffers: shared hit=146
Planning Time: 0.312 ms
JIT:
  Functions: 6
  Options: Inlining false, Optimization false, Expressions true, Deforming true
  Timing: Generation 0.443 ms, Inlining 0.000 ms, Optimization 0.144 ms, Emission 2.760 ms, Total 3.346 ms
Execution Time: 2107.907 ms

comment:4 by pramsey, 5 years ago

Seeing build failure in travis:

https://travis-ci.org/github/postgis/postgis/jobs/684751842

postgis_topology.c: In function 'GetRingEdges':

postgis_topology.c:5076:11: error: implicit declaration of function 'heap_form_tuple'; did you mean 'heap_lock_tuple'? [-Werror=implicit-function-declaration]

 5076 |   tuple = heap_form_tuple(funcctx->tuple_desc, ret, isnull);

      |           ^~~~~~~~~~~~~~~

      |           heap_lock_tuple

postgis_topology.c:5076:9: error: assignment to 'HeapTuple' {aka 'struct HeapTupleData *'} from 'int' makes pointer from integer without a cast [-Werror=int-conversion]

 5076 |   tuple = heap_form_tuple(funcctx->tuple_desc, ret, isnull);

      |         ^

postgis_topology.c: At top level:

comment:5 by Paul Ramsey <pramsey@…>, 5 years ago

In 10d2f810/git:

add missing pgsql header, references #4675

comment:6 by Algunenano, 4 years ago

Resolution: fixed
Status: closedreopened
Version: 3.0.xmaster

It seems this broke bessie32:

https://debbie.postgis.net/view/PostGIS/job/PostGIS_Worker_Run/label=bessie32/2018/console

11:24:51  regress/getringedges .. failed (diff expected obtained: /home/jenkins/tmp/pgis_reg_10c9dc2ac242c194763a47edab2b2487b42eb3a6/test_47_diff)
11:24:51 -----------------------------------------------------------------------------
11:24:51 --- regress/getringedges_expected	2020-05-11 01:55:21.793115000 -0500
11:24:51 +++ /home/jenkins/tmp/pgis_reg_10c9dc2ac242c194763a47edab2b2487b42eb3a6/test_47_out	2020-05-11 02:22:27.147195000 -0500
11:24:51 @@ -2,234 +2,234 @@
11:24:51  9
11:24:51  22
11:24:51  26
11:24:51 -R1|1|1
11:24:51 -R2|1|2
11:24:51 -R2|2|3
11:24:51 -R2|3|-3
11:24:51 -R3|1|3
11:24:51 -R3|2|-3
11:24:51 -R3|3|2
11:24:51 -R4|1|4
11:24:51 -R4|2|-5
11:24:51 -R4|3|5
11:24:51 -R4|4|-4
11:24:51 -R5|1|5
11:24:51 -R5|2|-4
11:24:51 -R5|3|4
11:24:51 -R5|4|-5
11:24:51 -R6|1|6
11:24:51 -R6|2|7
11:24:51 -R6|3|8
11:24:51 -R6|4|-15
11:24:51 -R6|5|-16
11:24:51 -R6|6|-14
11:24:51 -R6|7|-13
11:24:51 -R6|8|-12
11:24:51 -R6|9|22
11:24:51 -R6|10|21
11:24:51 -R7|1|7
11:24:51 -R7|2|8
11:24:51 -R7|3|-15
11:24:51 -R7|4|-16
11:24:51 -R7|5|-14
11:24:51 -R7|6|-13
11:24:51 -R7|7|-12
11:24:51 -R7|8|22
11:24:51 -R7|9|21
11:24:51 -R7|10|6
11:24:51 -R8|1|8
11:24:51 -R8|2|-15
11:24:51 -R8|3|-16
11:24:51 -R8|4|-14
11:24:51 -R8|5|-13
11:24:51 -R8|6|-12
11:24:51 -R8|7|22
11:24:51 -R8|8|21
11:24:51 -R8|9|6
11:24:51 -R8|10|7
11:24:51 -R9|1|9
11:24:51 -R9|2|19
11:24:51 -R9|3|-6
11:24:51 -R9|4|-21
11:24:51 -R10|1|10
11:24:51 -R10|2|-20
11:24:51 -R10|3|13
11:24:51 -R10|4|18
11:24:51 -R11|1|11
11:24:51 -R11|2|15
11:24:51 -R11|3|-8
11:24:51 -R11|4|-17
11:24:51 -R12|1|12
11:24:51 -R12|2|20
11:24:51 -R12|3|-9
11:24:51 -R12|4|-22
11:24:51 -R13|1|13
11:24:51 -R13|2|18
11:24:51 -R13|3|10
11:24:51 -R13|4|-20
11:24:51 -R14|1|14
11:24:51 -R14|2|16
11:24:51 -R14|3|-11
11:24:51 -R14|4|-18
11:24:51 -R15|1|15
11:24:51 -R15|2|-8
11:24:51 -R15|3|-17
11:24:51 -R15|4|11
11:24:51 -R16|1|16
11:24:51 -R16|2|-11
11:24:51 -R16|3|-18
11:24:51 -R16|4|14
11:24:51 -R17|1|17
11:24:51 -R17|2|-7
11:24:51 -R17|3|-19
11:24:51 -R17|4|-10
11:24:51 -R18|1|18
11:24:51 -R18|2|10
11:24:51 -R18|3|-20
11:24:51 -R18|4|13
11:24:51 -R19|1|19
11:24:51 -R19|2|-6
11:24:51 -R19|3|-21
11:24:51 -R19|4|9
11:24:51 -R20|1|20
11:24:51 -R20|2|-9
11:24:51 -R20|3|-22
11:24:51 -R20|4|12
11:24:51 -R21|1|21
11:24:51 -R21|2|6
11:24:51 -R21|3|7
11:24:51 -R21|4|8
11:24:51 -R21|5|-15
11:24:51 -R21|6|-16
11:24:51 -R21|7|-14
11:24:51 -R21|8|-13
11:24:51 -R21|9|-12
11:24:51 -R21|10|22
11:24:51 -R22|1|22
11:24:51 -R22|2|21
11:24:51 -R22|3|6
11:24:51 -R22|4|7
11:24:51 -R22|5|8
11:24:51 -R22|6|-15
11:24:51 -R22|7|-16
11:24:51 -R22|8|-14
11:24:51 -R22|9|-13
11:24:51 -R22|10|-12
11:24:51 -R25|1|25
11:24:51 -R25|2|-25
11:24:51 -R26|1|26
11:24:51 -R-1|1|-1
11:24:51 -R-2|1|-2
11:24:51 -R-3|1|-3
11:24:51 -R-3|2|2
11:24:51 -R-3|3|3
11:24:51 -R-4|1|-4
11:24:51 -R-4|2|4
11:24:51 -R-4|3|-5
11:24:51 -R-4|4|5
11:24:51 -R-5|1|-5
11:24:51 -R-5|2|5
11:24:51 -R-5|3|-4
11:24:51 -R-5|4|4
11:24:51 -R-6|1|-6
11:24:51 -R-6|2|-21
11:24:51 -R-6|3|9
11:24:51 -R-6|4|19
11:24:51 -R-7|1|-7
11:24:51 -R-7|2|-19
11:24:51 -R-7|3|-10
11:24:51 -R-7|4|17
11:24:51 -R-8|1|-8
11:24:51 -R-8|2|-17
11:24:51 -R-8|3|11
11:24:51 -R-8|4|15
11:24:51 -R-9|1|-9
11:24:51 -R-9|2|-22
11:24:51 -R-9|3|12
11:24:51 -R-9|4|20
11:24:51 -R-10|1|-10
11:24:51 -R-10|2|17
11:24:51 -R-10|3|-7
11:24:51 -R-10|4|-19
11:24:51 -R-11|1|-11
11:24:51 -R-11|2|-18
11:24:51 -R-11|3|14
11:24:51 -R-11|4|16
11:24:51 -R-12|1|-12
11:24:51 -R-12|2|22
11:24:51 -R-12|3|21
11:24:51 -R-12|4|6
11:24:51 -R-12|5|7
11:24:51 -R-12|6|8
11:24:51 -R-12|7|-15
11:24:51 -R-12|8|-16
11:24:51 -R-12|9|-14
11:24:51 -R-12|10|-13
11:24:51 -R-13|1|-13
11:24:51 -R-13|2|-12
11:24:51 -R-13|3|22
11:24:51 -R-13|4|21
11:24:51 -R-13|5|6
11:24:51 -R-13|6|7
11:24:51 -R-13|7|8
11:24:51 -R-13|8|-15
11:24:51 -R-13|9|-16
11:24:51 -R-13|10|-14
11:24:51 -R-14|1|-14
11:24:51 -R-14|2|-13
11:24:51 -R-14|3|-12
11:24:51 -R-14|4|22
11:24:51 -R-14|5|21
11:24:51 -R-14|6|6
11:24:51 -R-14|7|7
11:24:51 -R-14|8|8
11:24:51 -R-14|9|-15
11:24:51 -R-14|10|-16
11:24:51 -R-15|1|-15
11:24:51 -R-15|2|-16
11:24:51 -R-15|3|-14
11:24:51 -R-15|4|-13
11:24:51 -R-15|5|-12
11:24:51 -R-15|6|22
11:24:51 -R-15|7|21
11:24:51 -R-15|8|6
11:24:51 -R-15|9|7
11:24:51 -R-15|10|8
11:24:51 -R-16|1|-16
11:24:51 -R-16|2|-14
11:24:51 -R-16|3|-13
11:24:51 -R-16|4|-12
11:24:51 -R-16|5|22
11:24:51 -R-16|6|21
11:24:51 -R-16|7|6
11:24:51 -R-16|8|7
11:24:51 -R-16|9|8
11:24:51 -R-16|10|-15
11:24:51 -R-17|1|-17
11:24:51 -R-17|2|11
11:24:51 -R-17|3|15
11:24:51 -R-17|4|-8
11:24:51 -R-18|1|-18
11:24:51 -R-18|2|14
11:24:51 -R-18|3|16
11:24:51 -R-18|4|-11
11:24:51 -R-19|1|-19
11:24:51 -R-19|2|-10
11:24:51 -R-19|3|17
11:24:51 -R-19|4|-7
11:24:51 -R-20|1|-20
11:24:51 -R-20|2|13
11:24:51 -R-20|3|18
11:24:51 -R-20|4|10
11:24:51 -R-21|1|-21
11:24:51 -R-21|2|9
11:24:51 -R-21|3|19
11:24:51 -R-21|4|-6
11:24:51 -R-22|1|-22
11:24:51 -R-22|2|12
11:24:51 -R-22|3|20
11:24:51 -R-22|4|-9
11:24:51 -R-25|1|-25
11:24:51 -R-25|2|25
11:24:51 -R-26|1|-26
11:24:51 +R1|1|841859236
11:24:51 +R2|1|841859236
11:24:51 +R2|2|841859212
11:24:51 +R2|3|841859212
11:24:51 +R3|1|841859236
11:24:51 +R3|2|841859212
11:24:51 +R3|3|841859212
11:24:51 +R4|1|841859236
11:24:51 +R4|2|841859212
11:24:51 +R4|3|841859212
11:24:51 +R4|4|841859212
11:24:51 +R5|1|841859236
11:24:51 +R5|2|841859212
11:24:51 +R5|3|841859212
11:24:51 +R5|4|841859212
11:24:51 +R6|1|841859236
11:24:51 +R6|2|841859212
11:24:51 +R6|3|841859212
11:24:51 +R6|4|841859212
11:24:51 +R6|5|841859212
11:24:51 +R6|6|841859212
11:24:51 +R6|7|841859212
11:24:51 +R6|8|841859212
11:24:51 +R6|9|841859212
11:24:51 +R6|10|841859212
11:24:51 +R7|1|841859236
11:24:51 +R7|2|841859212
11:24:51 +R7|3|841859212
11:24:51 +R7|4|841859212
11:24:51 +R7|5|841859212
11:24:51 +R7|6|841859212
11:24:51 +R7|7|841859212
11:24:51 +R7|8|841859212
11:24:51 +R7|9|841859212
11:24:51 +R7|10|841859212
11:24:51 +R8|1|841859236
11:24:51 +R8|2|841859212
11:24:51 +R8|3|841859212
11:24:51 +R8|4|841859212
11:24:51 +R8|5|841859212
11:24:51 +R8|6|841859212
11:24:51 +R8|7|841859212
11:24:51 +R8|8|841859212
11:24:51 +R8|9|841859212
11:24:51 +R8|10|841859212
11:24:51 +R9|1|841859236
11:24:51 +R9|2|841859212
11:24:51 +R9|3|841859212
11:24:51 +R9|4|841859212
11:24:51 +R10|1|841859236
11:24:51 +R10|2|841859212
11:24:51 +R10|3|841859212
11:24:51 +R10|4|841859212
11:24:51 +R11|1|841859236
11:24:51 +R11|2|841859212
11:24:51 +R11|3|841859212
11:24:51 +R11|4|841859212
11:24:51 +R12|1|841859236
11:24:51 +R12|2|841859212
11:24:51 +R12|3|841859212
11:24:51 +R12|4|841859212
11:24:51 +R13|1|841859236
11:24:51 +R13|2|841859212
11:24:51 +R13|3|841859212
11:24:51 +R13|4|841859212
11:24:51 +R14|1|841859236
11:24:51 +R14|2|841859212
11:24:51 +R14|3|841859212
11:24:51 +R14|4|841859212
11:24:51 +R15|1|841859236
11:24:51 +R15|2|841859212
11:24:51 +R15|3|841859212
11:24:51 +R15|4|841859212
11:24:51 +R16|1|841859236
11:24:51 +R16|2|841859212
11:24:51 +R16|3|841859212
11:24:51 +R16|4|841859212
11:24:51 +R17|1|841859236
11:24:51 +R17|2|841859212
11:24:51 +R17|3|841859212
11:24:51 +R17|4|841859212
11:24:51 +R18|1|841859236
11:24:51 +R18|2|841859212
11:24:51 +R18|3|841859212
11:24:51 +R18|4|841859212
11:24:51 +R19|1|841859236
11:24:51 +R19|2|841859212
11:24:51 +R19|3|841859212
11:24:51 +R19|4|841859212
11:24:51 +R20|1|841859236
11:24:51 +R20|2|841859212
11:24:51 +R20|3|841859212
11:24:51 +R20|4|841859212
11:24:51 +R21|1|841859236
11:24:51 +R21|2|841859212
11:24:51 +R21|3|841859212
11:24:51 +R21|4|841859212
11:24:51 +R21|5|841859212
11:24:51 +R21|6|841859212
11:24:51 +R21|7|841859212
11:24:51 +R21|8|841859212
11:24:51 +R21|9|841859212
11:24:51 +R21|10|841859212
11:24:51 +R22|1|841859236
11:24:51 +R22|2|841859212
11:24:51 +R22|3|841859212
11:24:51 +R22|4|841859212
11:24:51 +R22|5|841859212
11:24:51 +R22|6|841859212
11:24:51 +R22|7|841859212
11:24:51 +R22|8|841859212
11:24:51 +R22|9|841859212
11:24:51 +R22|10|841859212
11:24:51 +R25|1|841859236
11:24:51 +R25|2|841859212
11:24:51 +R26|1|841859236
11:24:51 +R-1|1|841380004
11:24:51 +R-2|1|841380004
11:24:51 +R-3|1|841380004
11:24:51 +R-3|2|841379980
11:24:51 +R-3|3|841379980
11:24:51 +R-4|1|841380004
11:24:51 +R-4|2|841379980
11:24:51 +R-4|3|841379980
11:24:51 +R-4|4|841379980
11:24:51 +R-5|1|841380004
11:24:51 +R-5|2|841379980
11:24:51 +R-5|3|841379980
11:24:51 +R-5|4|841379980
11:24:51 +R-6|1|841380004
11:24:51 +R-6|2|841379980
11:24:51 +R-6|3|841379980
11:24:51 +R-6|4|841379980
11:24:51 +R-7|1|841380004
11:24:51 +R-7|2|841379980
11:24:51 +R-7|3|841379980
11:24:51 +R-7|4|841379980
11:24:51 +R-8|1|841380004
11:24:51 +R-8|2|841379980
11:24:51 +R-8|3|841379980
11:24:51 +R-8|4|841379980
11:24:51 +R-9|1|841380004
11:24:51 +R-9|2|841379980
11:24:51 +R-9|3|841379980
11:24:51 +R-9|4|841379980
11:24:51 +R-10|1|841380004
11:24:51 +R-10|2|841379980
11:24:51 +R-10|3|841379980
11:24:51 +R-10|4|841379980
11:24:51 +R-11|1|841380004
11:24:51 +R-11|2|841379980
11:24:51 +R-11|3|841379980
11:24:51 +R-11|4|841379980
11:24:51 +R-12|1|841380004
11:24:51 +R-12|2|841379980
11:24:51 +R-12|3|841379980
11:24:51 +R-12|4|841379980
11:24:51 +R-12|5|841379980
11:24:51 +R-12|6|841379980
11:24:51 +R-12|7|841379980
11:24:51 +R-12|8|841379980
11:24:51 +R-12|9|841379980
11:24:51 +R-12|10|841379980
11:24:51 +R-13|1|841380004
11:24:51 +R-13|2|841379980
11:24:51 +R-13|3|841379980
11:24:51 +R-13|4|841379980
11:24:51 +R-13|5|841379980
11:24:51 +R-13|6|841379980
11:24:51 +R-13|7|841379980
11:24:51 +R-13|8|841379980
11:24:51 +R-13|9|841379980
11:24:51 +R-13|10|841379980
11:24:51 +R-14|1|841380004
11:24:51 +R-14|2|841379980
11:24:51 +R-14|3|841379980
11:24:51 +R-14|4|841379980
11:24:51 +R-14|5|841379980
11:24:51 +R-14|6|841379980
11:24:51 +R-14|7|841379980
11:24:51 +R-14|8|841379980
11:24:51 +R-14|9|841379980
11:24:51 +R-14|10|841379980
11:24:51 +R-15|1|841380004
11:24:51 +R-15|2|841379980
11:24:51 +R-15|3|841379980
11:24:51 +R-15|4|841379980
11:24:51 +R-15|5|841379980
11:24:51 +R-15|6|841379980
11:24:51 +R-15|7|841379980
11:24:51 +R-15|8|841379980
11:24:51 +R-15|9|841379980
11:24:51 +R-15|10|841379980
11:24:51 +R-16|1|841380004
11:24:51 +R-16|2|841379980
11:24:51 +R-16|3|841379980
11:24:51 +R-16|4|841379980
11:24:51 +R-16|5|841379980
11:24:51 +R-16|6|841379980
11:24:51 +R-16|7|841379980
11:24:51 +R-16|8|841379980
11:24:51 +R-16|9|841379980
11:24:51 +R-16|10|841379980
11:24:51 +R-17|1|841380004
11:24:51 +R-17|2|841379980
11:24:51 +R-17|3|841379980
11:24:51 +R-17|4|841379980
11:24:51 +R-18|1|841380004
11:24:51 +R-18|2|841379980
11:24:51 +R-18|3|841379980
11:24:51 +R-18|4|841379980
11:24:51 +R-19|1|841380004
11:24:51 +R-19|2|841379980
11:24:51 +R-19|3|841379980
11:24:51 +R-19|4|841379980
11:24:51 +R-20|1|841380004
11:24:51 +R-20|2|841379980
11:24:51 +R-20|3|841379980
11:24:51 +R-20|4|841379980
11:24:51 +R-21|1|841380004
11:24:51 +R-21|2|841379980
11:24:51 +R-21|3|841379980
11:24:51 +R-21|4|841379980
11:24:51 +R-22|1|841380004
11:24:51 +R-22|2|841379980
11:24:51 +R-22|3|841379980
11:24:51 +R-22|4|841379980
11:24:51 +R-25|1|841380004
11:24:51 +R-25|2|841379980
11:24:51 +R-26|1|841380004
11:24:51  Topology 'city_data' dropped
11:24:51 -----------------------------------------------------------------------------

It might be (haven't checked) because it declares that it returns a

CREATE TYPE topology.GetFaceEdges_ReturnType AS (
  sequence integer,
  edge integer
);

Which are 2 int32 bytes, yet it's returning

	5074	  ret[0] = Int32GetDatum(state->curr);
 	5075	  ret[1] = Int64GetDatum(edge_id);

comment:7 by Sandro Santilli <strk@…>, 4 years ago

Resolution: fixed
Status: reopenedclosed

In 4d064ba/git:

edge_id is really a 32bit integer

At least until #3110 is done.

Should fix #4675

Note: See TracTickets for help on using tickets.