From c3d953991cc2ef7ef63da9f1d1b8f5d0d85e5d22 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Wed, 9 Sep 2026 15:24:36 +0200 Subject: [PATCH] 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. This also makes the version of the proj library osm2pgsql was compiled with available in the Lua config as the "osm2pgsql.proj_version" string variable. It is set to '[disabled]' if osm2pgsql was compiled without proj support. It is needed for this so we can disable the test if proj is not available. --- src/db-copy.cpp | 5 ++ src/db-copy.hpp | 12 +++- src/flex-table.cpp | 56 ++++++---------- src/flex-table.hpp | 4 +- src/output-flex.cpp | 1 + src/pgsql-helper.cpp | 41 ------------ src/pgsql-helper.hpp | 9 --- src/table.cpp | 19 ++---- .../bdd/flex/invalid-after-projection.feature | 67 +++++++++++++++++++ tests/test-output-flex-schema.cpp | 9 --- 10 files changed, 111 insertions(+), 112 deletions(-) create mode 100644 tests/bdd/flex/invalid-after-projection.feature diff --git a/src/db-copy.cpp b/src/db-copy.cpp index 1a7dedec5..e28e744a2 100644 --- a/src/db-copy.cpp +++ b/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)); diff --git a/src/db-copy.hpp b/src/db-copy.hpp index 3a9d3f05e..84faf8515 100644 --- a/src/db-copy.hpp +++ b/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; }; /** diff --git a/src/flex-table.cpp b/src/flex-table.cpp index 263a9cedc..fbd16f704 100644 --- a/src/flex-table.cpp +++ b/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()) { diff --git a/src/flex-table.hpp b/src/flex-table.hpp index 663375327..be932f6c7 100644 --- a/src/flex-table.hpp +++ b/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( 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) { } diff --git a/src/output-flex.cpp b/src/output-flex.cpp index 04b735d8e..85c65db35 100644 --- a/src/output-flex.cpp +++ b/src/output-flex.cpp @@ -1409,6 +1409,7 @@ void output_flex_t::init_lua(std::string const &filename, setup_lua_environment(lua_state(), filename, get_options()->append); luaX_add_table_int(lua_state(), "stage", 1); + luaX_add_table_str(lua_state(), "proj_version", get_proj_version()); lua_pushliteral(lua_state(), "properties"); lua_createtable(lua_state(), 0, (int)properties.size()); diff --git a/src/pgsql-helper.cpp b/src/pgsql-helper.cpp index d859b13a3..3a231d74f 100644 --- a/src/pgsql-helper.cpp +++ b/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) { diff --git a/src/pgsql-helper.hpp b/src/pgsql-helper.hpp index 4ab4abd51..daf1f1d1a 100644 --- a/src/pgsql-helper.hpp +++ b/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); diff --git a/src/table.cpp b/src/table.cpp index 6b65eb9d5..ed3e17935 100644 --- a/src/table.cpp +++ b/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,10 @@ void table_t::generate_copy_column_list() joiner.add("way"); m_target->set_rows(joiner()); + + if (m_srid != "4326") { + m_target->set_conditions("ST_IsValid(way)"); + } } void table_t::stop(bool updateable, bool enable_hstore_index, @@ -185,11 +184,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 +211,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 */ diff --git a/tests/bdd/flex/invalid-after-projection.feature b/tests/bdd/flex/invalid-after-projection.feature new file mode 100644 index 000000000..32946e53a --- /dev/null +++ b/tests/bdd/flex/invalid-after-projection.feature @@ -0,0 +1,67 @@ +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 srid = 4326 + + if osm2pgsql.proj_version ~= '[disabled]' then + srid = 3031 + end + + local polygons = osm2pgsql.define_table({ + name = 'osm2pgsql_test_polygon', + ids = { type = 'way', id_column = 'osm_id' }, + columns = { + { column = 'geom', type = 'polygon', projection = srid }, + } + }) + + if osm2pgsql.proj_version ~= '[disabled]' then + function osm2pgsql.process_way(object) + polygons:insert({ + geom = object:as_polygon() + }) + end + end + """ + When running osm2pgsql flex + Then table osm2pgsql_test_polygon has 0 rows + diff --git a/tests/test-output-flex-schema.cpp b/tests/test-output-flex-schema.cpp index 7dd6a8587..14b216b16 100644 --- a/tests/test-output-flex-schema.cpp +++ b/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'")); }