Replace geometry check trigger by WHERE condition on COPY

Since version 12 PostgreSQL supports a WHERE condition on the
COPY command. Use this to add a geometry check when adding data
to the database instead of the trigger we used before.

This makes the code simpler and doesn't clutter the database with
triggers.

If you have already imported a database with an older version of
osm2pgsql and want to use this new version, that will still work. But
the original triggers will still be there so there is some extra work
done for each update. Simply remove the triggers and trigger functions
from the database with commands of the form
    DROP TRIGGER *_osm2pgsql_valid ON *;
    DROP FUNCTION *_osm2pgsql_valid;

PostgreSQL 12 is currently the oldest database version we support.

Author: joto

src/db-copy.cpp

@@ -220,6 +220,11 @@ void db_copy_thread_t::thread_t::start_copy(
                        target->rows());
     }
 
+    if (!target->conditions().empty()) {
+        fmt::format_to(std::back_inserter(sql), FMT_STRING(" WHERE {}"),
+                       target->conditions());
+    }
+
     sql.push_back('\0');
     m_db_connection.copy_start(to_string(sql));
 

src/db-copy.hpp

@@ -34,9 +34,9 @@ class db_target_descr_t
 {
 public:
     db_target_descr_t(std::string schema, std::string name, std::string id,
-                      std::string rows = {})
+                      std::string rows = {}, std::string conditions = {})
     : m_schema(std::move(schema)), m_name(std::move(name)), m_id(std::move(id)),
-      m_rows(std::move(rows))
+      m_rows(std::move(rows)), m_conditions(std::move(conditions))
     {
         assert(!m_schema.empty());
         assert(!m_name.empty());
@@ -46,9 +46,15 @@ class db_target_descr_t
     std::string const &name() const noexcept { return m_name; }
     std::string const &id() const noexcept { return m_id; }
     std::string const &rows() const noexcept { return m_rows; }
+    std::string const &conditions() const noexcept { return m_conditions; }
 
     void set_rows(std::string rows) { m_rows = std::move(rows); }
 
+    void set_conditions(std::string conditions)
+    {
+        m_conditions = std::move(conditions);
+    }
+
     /**
      * Check if the buffer would use exactly the same copy operation.
      */
@@ -68,6 +74,8 @@ class db_target_descr_t
     std::string m_id;
     /// Comma-separated list of rows for copy operation (when empty: all rows)
     std::string m_rows;
+    /// Conditions for the COPY command.
+    std::string m_conditions;
 };
 
 /**

src/flex-table.cpp

@@ -222,6 +222,27 @@ std::string flex_table_t::build_sql_column_list() const
     return joiner();
 }
 
+std::string flex_table_t::build_sql_copy_condition() const
+{
+    assert(!m_columns.empty());
+
+    std::string checks;
+
+    for (auto const &column : m_columns) {
+        if (column.is_geometry_column() && column.needs_isvalid()) {
+            checks.append(fmt::format(
+                R"(("{0}" IS NULL OR ST_IsValid("{0}")) AND )", column.name()));
+        }
+    }
+
+    if (!checks.empty()) {
+        // remove last " AND "
+        checks.resize(checks.size() - 5);
+    }
+
+    return checks;
+}
+
 std::string flex_table_t::build_sql_create_id_index() const
 {
     if (m_primary_key_index) {
@@ -269,30 +290,6 @@ bool flex_table_t::with_id_cache() const noexcept { return m_with_id_cache; }
 
 namespace {
 
-void enable_check_trigger(pg_conn_t const &db_connection,
-                          flex_table_t const &table)
-{
-    std::string checks;
-
-    for (auto const &column : table.columns()) {
-        if (column.is_geometry_column() && column.needs_isvalid()) {
-            checks.append(fmt::format(
-                R"((NEW."{0}" IS NULL OR ST_IsValid(NEW."{0}")) AND )",
-                column.name()));
-        }
-    }
-
-    if (checks.empty()) {
-        return;
-    }
-
-    // remove last " AND "
-    checks.resize(checks.size() - 5);
-
-    create_geom_check_trigger(db_connection, table.schema(), table.name(),
-                              checks);
-}
-
 } // anonymous namespace
 
 void table_connection_t::start(pg_conn_t const &db_connection,
@@ -311,8 +308,6 @@ void table_connection_t::start(pg_conn_t const &db_connection,
             table().cluster_by_geom() ? flex_table_t::table_type::interim
                                       : flex_table_t::table_type::permanent,
             table().full_name()));
-
-        enable_check_trigger(db_connection, table());
     }
 
     table().prepare(db_connection);
@@ -328,11 +323,6 @@ void table_connection_t::stop(pg_conn_t const &db_connection, bool updateable,
     }
 
     if (table().cluster_by_geom()) {
-        if (table().geom_column().needs_isvalid()) {
-            drop_geom_check_trigger(db_connection, table().schema(),
-                                    table().name());
-        }
-
         log_info("Clustering table '{}' by geometry...", table().name());
 
         db_connection.exec(table().build_sql_create_table(
@@ -354,10 +344,6 @@ void table_connection_t::stop(pg_conn_t const &db_connection, bool updateable,
         db_connection.exec(R"(ALTER TABLE {} RENAME TO "{}")",
                            table().full_tmp_name(), table().name());
         m_id_index_created = false;
-
-        if (updateable) {
-            enable_check_trigger(db_connection, table());
-        }
     }
 
     if (table().indexes().empty()) {

src/flex-table.hpp

@@ -157,6 +157,8 @@ class flex_table_t
 
     std::string build_sql_column_list() const;
 
+    std::string build_sql_copy_condition() const;
+
     std::string build_sql_create_id_index() const;
 
     /// Does this table take objects of the specified type?
@@ -288,7 +290,7 @@ class table_connection_t
     : m_proj(reprojection_t::create_projection(table->srid())), m_table(table),
       m_target(std::make_shared<db_target_descr_t>(
           table->schema(), table->name(), table->id_column_names(),
-          table->build_sql_column_list())),
+          table->build_sql_column_list(), table->build_sql_copy_condition())),
       m_copy_mgr(copy_thread)
     {
     }

src/pgsql-helper.cpp

@@ -29,47 +29,6 @@ idlist_t get_ids_from_result(pg_result_t const &result)
     return ids;
 }
 
-void create_geom_check_trigger(pg_conn_t const &db_connection,
-                               std::string const &schema,
-                               std::string const &table,
-                               std::string const &condition)
-{
-    std::string const func_name =
-        qualified_name(schema, table + "_osm2pgsql_valid");
-
-    db_connection.exec("CREATE OR REPLACE FUNCTION {}()\n"
-                       "RETURNS TRIGGER AS $$\n"
-                       "BEGIN\n"
-                       "  IF {} THEN \n"
-                       "    RETURN NEW;\n"
-                       "  END IF;\n"
-                       "  RETURN NULL;\n"
-                       "END;"
-                       "$$ LANGUAGE plpgsql",
-                       func_name, condition);
-
-    db_connection.exec("CREATE TRIGGER \"{}\""
-                       " BEFORE INSERT OR UPDATE"
-                       " ON {}"
-                       " FOR EACH ROW EXECUTE PROCEDURE"
-                       " {}()",
-                       table + "_osm2pgsql_valid",
-                       qualified_name(schema, table), func_name);
-}
-
-void drop_geom_check_trigger(pg_conn_t const &db_connection,
-                             std::string const &schema,
-                             std::string const &table)
-{
-    std::string const func_name =
-        qualified_name(schema, table + "_osm2pgsql_valid");
-
-    db_connection.exec(R"(DROP TRIGGER "{}" ON {})", table + "_osm2pgsql_valid",
-                       qualified_name(schema, table));
-
-    db_connection.exec("DROP FUNCTION IF EXISTS {} ()", func_name);
-}
-
 void analyze_table(pg_conn_t const &db_connection, std::string const &schema,
                    std::string const &name)
 {

src/pgsql-helper.hpp

@@ -27,15 +27,6 @@ class pg_result_t;
  */
 idlist_t get_ids_from_result(pg_result_t const &result);
 
-void create_geom_check_trigger(pg_conn_t const &db_connection,
-                               std::string const &schema,
-                               std::string const &table,
-                               std::string const &condition);
-
-void drop_geom_check_trigger(pg_conn_t const &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

@@ -129,11 +129,6 @@ void table_t::start(connection_params_t const &connection_params,
 
         //create the table
         m_db_connection->exec(sql);
-
-        if (m_srid != "4326") {
-            create_geom_check_trigger(*m_db_connection, m_target->schema(),
-                                      m_target->name(), "ST_IsValid(NEW.way)");
-        }
     }
 
     prepare();
@@ -172,6 +167,7 @@ void table_t::generate_copy_column_list()
     joiner.add("way");
 
     m_target->set_rows(joiner());
+    m_target->set_conditions("way IS NULL OR ST_IsValid(way)");
 }
 
 void table_t::stop(bool updateable, bool enable_hstore_index,
@@ -185,11 +181,6 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
         qualified_name(m_target->schema(), m_target->name() + "_tmp");
 
     if (!m_append) {
-        if (m_srid != "4326") {
-            drop_geom_check_trigger(*m_db_connection, m_target->schema(),
-                                    m_target->name());
-        }
-
         log_info("Clustering table '{}' by geometry...", m_target->name());
 
         std::string const sql =
@@ -217,11 +208,6 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
             m_db_connection->exec("CREATE INDEX ON {} USING BTREE (osm_id) {}",
                                   qual_name,
                                   tablespace_clause(table_space_index));
-            if (m_srid != "4326") {
-                create_geom_check_trigger(*m_db_connection, m_target->schema(),
-                                          m_target->name(),
-                                          "ST_IsValid(NEW.way)");
-            }
         }
 
         /* Create hstore index if selected */

tests/bdd/flex/invalid-after-projection.feature

@@ -0,0 +1,58 @@
+Feature: A valid geometry that is projected can become invalid
+
+    Scenario: Valid geometry must end up in output table
+        Given the OSM data
+            """
+            n10 t2020-01-02T03:04:05Z x0 y50
+            n11 t2020-01-02T03:04:05Z x50 y50
+            n12 t2020-01-02T03:04:05Z x100 y50
+            n13 t2020-01-02T03:04:05Z x100 y50.1
+            n14 t2020-01-02T03:04:05Z x0 y50.1
+            w20 t2020-01-02T03:04:06Z Tlanduse=forest Nn10,n11,n12,n13,n14,n10
+            """
+        And the lua style
+            """
+            local polygons = osm2pgsql.define_table({
+                name = 'osm2pgsql_test_polygon',
+                ids = { type = 'way', id_column = 'osm_id' },
+                columns = {
+                    { column = 'geom', type = 'polygon', projection = 4326 },
+                }
+            })
+            function osm2pgsql.process_way(object)
+                polygons:insert({
+                    geom = object:as_polygon()
+                })
+            end
+            """
+        When running osm2pgsql flex
+        Then table osm2pgsql_test_polygon has 1 rows
+
+    Scenario: Invalid geometry after projection must not end up in output table
+        Given the OSM data
+            """
+            n10 t2020-01-02T03:04:05Z x0 y50
+            n11 t2020-01-02T03:04:05Z x50 y50
+            n12 t2020-01-02T03:04:05Z x100 y50
+            n13 t2020-01-02T03:04:05Z x100 y50.1
+            n14 t2020-01-02T03:04:05Z x0 y50.1
+            w20 t2020-01-02T03:04:06Z Tlanduse=forest Nn10,n11,n12,n13,n14,n10
+            """
+        And the lua style
+            """
+            local polygons = osm2pgsql.define_table({
+                name = 'osm2pgsql_test_polygon',
+                ids = { type = 'way', id_column = 'osm_id' },
+                columns = {
+                    { column = 'geom', type = 'polygon', projection = 3031 },
+                }
+            })
+            function osm2pgsql.process_way(object)
+                polygons:insert({
+                    geom = object:as_polygon()
+                })
+            end
+            """
+        When running osm2pgsql flex
+        Then table osm2pgsql_test_polygon has 0 rows
+

tests/test-output-flex-schema.cpp

@@ -37,13 +37,4 @@ TEST_CASE("config with schema should work")
             conn.get_count("pg_catalog.pg_tables", "schemaname = 'myschema'"));
 
     REQUIRE(7103 == conn.get_count("myschema.osm2pgsql_test_line"));
-
-    REQUIRE(1 ==
-            conn.get_count("pg_catalog.pg_proc",
-                           "proname = 'osm2pgsql_test_line_osm2pgsql_valid'"));
-
-    REQUIRE(1 == conn.get_count("pg_catalog.pg_trigger"));
-    REQUIRE(1 ==
-            conn.get_count("pg_catalog.pg_trigger",
-                           "tgname = 'osm2pgsql_test_line_osm2pgsql_valid'"));
 }