Merge pull request #1484 from joto/geom-valid-check-trigger

Improved geometry IsValid check

Author: lonvia

src/flex-table-column.hpp

@@ -87,6 +87,18 @@ class flex_table_column_t
                (m_type <= table_column_type::multipolygon);
     }
 
+    /**
+     * Do we need an ST_IsValid() check in the database for this geometry
+     * column? If the SRID is 4326 the geometry validity is already assured
+     * by libosmium, so we don't need it. And Point geometries are always
+     * valid.
+     */
+    bool needs_isvalid() const noexcept
+    {
+        assert(is_geometry_column());
+        return m_srid != 4326 && m_type != table_column_type::point;
+    }
+
     std::string const &type_name() const noexcept { return m_type_name; }
 
     bool not_null() const noexcept { return m_not_null; }

src/flex-table.cpp

@@ -176,6 +176,13 @@ void table_connection_t::start(bool append)
             table().has_geom_column() ? flex_table_t::table_type::interim
                                       : flex_table_t::table_type::permanent,
             table().full_name()));
+
+        if (table().has_geom_column() &&
+            table().geom_column().needs_isvalid()) {
+            create_geom_check_trigger(m_db_connection.get(), table().schema(),
+                                      table().name(),
+                                      table().geom_column().name());
+        }
     }
 
     prepare();
@@ -195,6 +202,11 @@ void table_connection_t::stop(bool updateable, bool append)
     util::timer_t timer;
 
     if (table().has_geom_column()) {
+        if (table().geom_column().needs_isvalid()) {
+            drop_geom_check_trigger(m_db_connection.get(), table().schema(),
+                                    table().name());
+        }
+
         log_info("Clustering table '{}' by geometry...", table().name());
 
         // Notices about invalid geometries are expected and can be ignored
@@ -207,14 +219,6 @@ void table_connection_t::stop(bool updateable, bool append)
         std::string sql = "INSERT INTO {} SELECT * FROM {}"_format(
             table().full_tmp_name(), table().full_name());
 
-        if (table().geom_column().srid() != 4326) {
-            // libosmium assures validity of geometries in 4326.
-            // Transformation to another projection could make the geometry
-            // invalid. Therefore add a filter to drop those.
-            sql += " WHERE ST_IsValid(\"{}\")"_format(
-                table().geom_column().name());
-        }
-
         auto const postgis_version = get_postgis_version(*m_db_connection);
 
         sql += " ORDER BY ";
@@ -257,7 +261,8 @@ void table_connection_t::stop(bool updateable, bool append)
     if (updateable && table().has_id_column()) {
         create_id_index();
 
-        if (table().has_geom_column() && table().geom_column().srid() != 4326) {
+        if (table().has_geom_column() &&
+            table().geom_column().needs_isvalid()) {
             create_geom_check_trigger(m_db_connection.get(), table().schema(),
                                       table().name(),
                                       table().geom_column().name());

src/pgsql-helper.cpp

@@ -55,6 +55,18 @@ void create_geom_check_trigger(pg_conn_t *db_connection,
                         qualified_name(schema, table), func_name));
 }
 
+void drop_geom_check_trigger(pg_conn_t *db_connection,
+                             std::string const &schema,
+                             std::string const &table)
+{
+    std::string func_name = qualified_name(schema, table + "_osm2pgsql_valid");
+
+    db_connection->exec("DROP TRIGGER \"{}\" ON {};"_format(
+        table + "_osm2pgsql_valid", qualified_name(schema, table)));
+
+    db_connection->exec("DROP FUNCTION IF EXISTS {} ();"_format(func_name));
+}
+
 void analyze_table(pg_conn_t const &db_connection, std::string const &schema,
                    std::string const &name)
 {

src/pgsql-helper.hpp

@@ -31,6 +31,10 @@ void create_geom_check_trigger(pg_conn_t *db_connection,
                                std::string const &table,
                                std::string const &geom_column);
 
+void drop_geom_check_trigger(pg_conn_t *db_connection,
+                             std::string const &schema,
+                             std::string const &table);
+
 void analyze_table(pg_conn_t const &db_connection, std::string const &schema,
                    std::string const &name);
 

src/table.cpp

@@ -132,6 +132,11 @@ void table_t::start(std::string const &conninfo, std::string const &table_space)
 
         //create the table
         m_sql_conn->exec(sql);
+
+        if (m_srid != "4326") {
+            create_geom_check_trigger(m_sql_conn.get(), m_target->schema,
+                                      m_target->name, "way");
+        }
     }
 
     prepare();
@@ -185,6 +190,11 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
     if (!m_append) {
         util::timer_t timer;
 
+        if (m_srid != "4326") {
+            drop_geom_check_trigger(m_sql_conn.get(), m_target->schema,
+                                    m_target->name);
+        }
+
         log_info("Clustering table '{}' by geometry...", m_target->name);
 
         // Notices about invalid geometries are expected and can be ignored
@@ -195,13 +205,6 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
             "CREATE TABLE {} {} AS SELECT * FROM {}"_format(
                 qual_tmp_name, m_table_space, qual_name);
 
-        if (m_srid != "4326") {
-            // libosmium assures validity of geometries in 4326.
-            // Transformation to another projection could make the geometry
-            // invalid. Therefore add a filter to drop those.
-            sql += " WHERE ST_IsValid(way)";
-        }
-
         auto const postgis_version = get_postgis_version(*m_sql_conn);
 
         sql += " ORDER BY ";

tests/data/test_output_flex_schema.lua

@@ -1,7 +1,7 @@
 
-local dtable = osm2pgsql.define_node_table('osm2pgsql_test_point', {
+local dtable = osm2pgsql.define_way_table('osm2pgsql_test_line', {
     { column = 'tags', type = 'hstore' },
-    { column = 'geom', type = 'point' },
+    { column = 'geom', type = 'linestring' },
 }, { schema = 'myschema' })
 
 local delete_keys = {
@@ -12,13 +12,14 @@ local delete_keys = {
 
 local clean_tags = osm2pgsql.make_clean_tags_func(delete_keys)
 
-function osm2pgsql.process_node(object)
+function osm2pgsql.process_way(object)
     if clean_tags(object.tags) then
         return
     end
 
     dtable:add_row({
-        tags = object.tags
+        tags = object.tags,
+        geom = { create = 'line' }
     })
 end
 

tests/test-output-flex-schema.cpp

@@ -29,14 +29,14 @@ TEST_CASE("config with schema should work")
     REQUIRE(1 == conn.get_count("pg_namespace", "nspname = 'myschema'"));
     REQUIRE(1 == conn.get_count("pg_tables", "schemaname = 'myschema'"));
 
-    REQUIRE(1362 == conn.get_count("myschema.osm2pgsql_test_point"));
+    REQUIRE(7103 == conn.get_count("myschema.osm2pgsql_test_line"));
 
     REQUIRE(1 ==
             conn.get_count("pg_proc",
-                           "proname = 'osm2pgsql_test_point_osm2pgsql_valid'"));
+                           "proname = 'osm2pgsql_test_line_osm2pgsql_valid'"));
 
     REQUIRE(1 == conn.get_count("pg_trigger"));
     REQUIRE(1 ==
             conn.get_count("pg_trigger",
-                           "tgname = 'osm2pgsql_test_point_osm2pgsql_valid'"));
+                           "tgname = 'osm2pgsql_test_line_osm2pgsql_valid'"));
 }