@lonvia commented on this pull request.
The mix of tile expiry and recursive query is interesting. I actually think
that the solution is simpler because of your use of tile-based expiry instead
of way-based. I did an implementation with way-based expiry for
[waymarkedtrails](https://github.com/waymarkedtrails/osgende/blob/master/osgende/lines/segments.py)
and only got it to work by tracking the IDs of the source ways for each merged
way. That would make this implementation significantly more complex.
ST_LineMerge has a few quirks which might lead to unexpected results but that
is something that can be easily addressed later.
Performance stands an falls with the complexity of the merged ways, which in
turn depends on the source table and the grouping. I'd like to run the import
query on a planet to see how long it takes. That should give us a ballpark
figure how expensive merging is.
> + // endpoint equality, not ST_Intersects, to avoid deleting unrelated
+ // same-group lines that cross a component. A second pass over the changed
+ // region cleans up outputs of components that disappeared entirely (all
+ // their lines deleted), which leave no reached nodes behind; that pass is
+ // self-correcting because anything it removes either vanished or has a
+ // surviving line in the region and is regenerated below.
+ timer(m_timer_delete).start();
+ auto deleted = dbexec(R"(
+DELETE FROM {dest} d
+ USING _glm_nodes n
+ WHERE {group_join_dn}
+ AND ( (ST_X(ST_StartPoint(d."{geom_column}")) = n.x
+ AND ST_Y(ST_StartPoint(d."{geom_column}")) = n.y)
+ OR (ST_X(ST_EndPoint(d."{geom_column}")) = n.x
+ AND ST_Y(ST_EndPoint(d."{geom_column}")) = n.y) )
+)");
Is this delete really necessary? Are there any cases of updating that wouldn't
be covered by the DELETE below in line335?
> + }
+}
+
+void gen_grouped_linemerge_t::process_create()
+{
+ if (get_params().get_bool("create_indexes", true)) {
+ log_gen("Creating endpoint indexes on source table...");
+ dbexec(
+ R"(CREATE INDEX IF NOT EXISTS "{idx_startpt}" ON {src} USING
btree)"
+ R"( (ST_X(ST_StartPoint("{geom_column}")),)"
+ R"( ST_Y(ST_StartPoint("{geom_column}"))) {index_predicate})");
+ dbexec(
+ R"(CREATE INDEX IF NOT EXISTS "{idx_endpt}" ON {src} USING btree)"
+ R"( (ST_X(ST_EndPoint("{geom_column}")),)"
+ R"( ST_Y(ST_EndPoint("{geom_column}"))) {index_predicate})");
+ }
I understand your reasoning for an extra index here but I think this still
could be a geometry index on the points, i.e.
```
CREATE INDEX ... USING gist(ST_StartPoint(geometry))
```
The you can use ST_Intersect on the start and end points below making the code
a lot more readable.
> + CASE WHEN ST_X(ST_StartPoint(l."{geom_column}")) = n.x
+ AND ST_Y(ST_StartPoint(l."{geom_column}")) = n.y
+ THEN ST_X(ST_EndPoint(l."{geom_column}"))
+ ELSE ST_X(ST_StartPoint(l."{geom_column}")) END,
+ CASE WHEN ST_X(ST_StartPoint(l."{geom_column}")) = n.x
+ AND ST_Y(ST_StartPoint(l."{geom_column}")) = n.y
+ THEN ST_Y(ST_EndPoint(l."{geom_column}"))
+ ELSE ST_Y(ST_StartPoint(l."{geom_column}")) END
+ FROM nodes n
+ JOIN {src} l
+ ON {group_join}
+ AND ( (ST_X(ST_StartPoint(l."{geom_column}")) = n.x
+ AND ST_Y(ST_StartPoint(l."{geom_column}")) = n.y)
+ OR (ST_X(ST_EndPoint(l."{geom_column}")) = n.x
+ AND ST_Y(ST_EndPoint(l."{geom_column}")) = n.y) )
+ AND {where}
One way to make this recursive CTE cheaper is to join here only with nodes
added in the last recursion. You need to be very, very careful though not to
create endless loops when doing that. So maybe an optimization for later rather
than now.
--
Reply to this email directly or view it on GitHub:
https://github.com/osm2pgsql-dev/osm2pgsql/pull/2482#pullrequestreview-4437753493
You are receiving this because you are subscribed to this thread.
Message ID: <osm2pgsql-dev/osm2pgsql/pull/2482/review/[email protected]>_______________________________________________
Tile-serving mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/tile-serving
