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'")); }