Opened 5 years ago
Closed 5 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:
- A PL/Pgsql version that is hit by running topology.GetRingEdges
- 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 , 5 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:2 by , 5 years ago
Milestone: | → PostGIS 3.1.0 |
---|---|
Type: | defect → enhancement |
comment:3 by , 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 , 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:6 by , 5 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Version: | 3.0.x → master |
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);
In ecae381/git: