Find way dependencies faster by doing a single lookup

For each changed way osm2pgsql has to find all relations referencing
that way and make sure they are updated as well. This was done by doing
one SQL query per way which is quite inefficient. Instead the new code
collects all way ids and does only one query for the parent relations
after all ways have been read.

This commit also makes sure that ways which have been in the input file
are removed from the pending way tracker so that they are not processed
again later.

Author: joto

src/dependency-manager.cpp

@@ -20,13 +20,7 @@ void full_dependency_manager_t::node_changed(osmid_t id)
 
 void full_dependency_manager_t::way_changed(osmid_t id)
 {
-    if (m_ways_pending_tracker.get(id)) {
-        return;
-    }
-
-    for (auto const rel_id : m_object_store->get_rels_by_way(id)) {
-        m_rels_pending_tracker.set(rel_id);
-    }
+    m_changed_ways.set(id);
 }
 
 void full_dependency_manager_t::after_nodes()
@@ -38,12 +32,55 @@ void full_dependency_manager_t::after_nodes()
     m_object_store->get_node_parents(m_changed_nodes, &m_ways_pending_tracker,
                                      &m_rels_pending_tracker);
     m_changed_nodes.clear();
+}
+
+static osmium::index::IdSetSmall<osmid_t>
+set_diff(osmium::index::IdSetSmall<osmid_t> const &set,
+         osmium::index::IdSetSmall<osmid_t> const &to_be_removed)
+{
+    osmium::index::IdSetSmall<osmid_t> new_set;
+
+    for (auto const id : set) {
+        if (!to_be_removed.get_binary_search(id)) {
+            new_set.set(id);
+        }
+    }
+
+    return new_set;
+}
+
+void full_dependency_manager_t::after_ways()
+{
+    if (!m_changed_ways.empty()) {
+        if (!m_ways_pending_tracker.empty()) {
+            // Remove ids from changed ways in the input data from
+            // m_ways_pending_tracker, because they have already been processed.
+            m_ways_pending_tracker =
+                set_diff(m_ways_pending_tracker, m_changed_ways);
 
-    for (auto const way_id : m_ways_pending_tracker) {
-        for (auto const rel_id : m_object_store->get_rels_by_way(way_id)) {
-            m_rels_pending_tracker.set(rel_id);
+            // Add the list of pending way ids to the list of changed ways,
+            // because we need the parents for them, too.
+            m_changed_ways.merge_sorted(m_ways_pending_tracker);
         }
+
+        m_object_store->get_way_parents(m_changed_ways,
+                                        &m_rels_pending_tracker);
+
+        m_changed_ways.clear();
+        return;
     }
+
+    if (!m_ways_pending_tracker.empty()) {
+        m_object_store->get_way_parents(m_ways_pending_tracker,
+                                        &m_rels_pending_tracker);
+    }
+}
+
+void full_dependency_manager_t::mark_parent_relations_as_pending(
+    osmium::index::IdSetSmall<osmid_t> const &way_ids)
+{
+    assert(m_rels_pending_tracker.empty());
+    m_object_store->get_way_parents(way_ids, &m_rels_pending_tracker);
 }
 
 bool full_dependency_manager_t::has_pending() const noexcept

src/dependency-manager.hpp

@@ -53,6 +53,12 @@ class dependency_manager_t
     virtual void way_changed(osmid_t) {}
 
     virtual void after_nodes() {}
+    virtual void after_ways() {}
+
+    virtual void mark_parent_relations_as_pending(
+        osmium::index::IdSetSmall<osmid_t> const & /*way_ids*/)
+    {
+    }
 
     /// Are there pending objects that need to be processed?
     virtual bool has_pending() const noexcept { return false; }
@@ -98,6 +104,10 @@ class full_dependency_manager_t : public dependency_manager_t
     void way_changed(osmid_t id) override;
 
     void after_nodes() override;
+    void after_ways() override;
+
+    void mark_parent_relations_as_pending(
+        osmium::index::IdSetSmall<osmid_t> const &ids) override;
 
     bool has_pending() const noexcept override;
 
@@ -126,6 +136,15 @@ class full_dependency_manager_t : public dependency_manager_t
      */
     osmium::index::IdSetSmall<osmid_t> m_changed_nodes;
 
+    /**
+     * In append mode all new and changed ways will be added to this. After
+     * all ways are read this is used to figure out which parent relations
+     * reference these ways. Deleted ways are not stored in here, because all
+     * relations that referenced deleted ways must be in the change file, too,
+     * and so we don't have to find out which ones they are.
+     */
+    osmium::index::IdSetSmall<osmid_t> m_changed_ways;
+
     osmium::index::IdSetSmall<osmid_t> m_ways_pending_tracker;
     osmium::index::IdSetSmall<osmid_t> m_rels_pending_tracker;
 };

src/middle-pgsql.cpp

@@ -127,8 +127,6 @@ middle_pgsql_t::table_desc::table_desc(options_t const &options,
     m_copy_target->id = "id";
 
     if (options.with_forward_dependencies) {
-        m_prepare_fw_dep_lookups =
-            build_sql(options, ts.prepare_fw_dep_lookups);
         m_create_fw_dep_indexes = build_sql(options, ts.create_fw_dep_indexes);
     }
 }
@@ -859,9 +857,57 @@ INSERT INTO osm2pgsql_changed_relations
               parent_ways->size(), parent_relations->size());
 }
 
-idlist_t middle_pgsql_t::get_rels_by_way(osmid_t osm_id)
+void middle_pgsql_t::get_way_parents(
+    osmium::index::IdSetSmall<osmid_t> const &changed_ways,
+    osmium::index::IdSetSmall<osmid_t> *parent_relations) const
 {
-    return get_ids_from_db(&m_db_connection, "mark_rels_by_way", osm_id);
+    util::timer_t timer;
+
+    auto const num_relations_referenced_by_nodes = parent_relations->size();
+
+    m_db_connection.exec("BEGIN");
+    m_db_connection.exec("CREATE TEMP TABLE osm2pgsql_changed_ways"
+                         " (id int8 NOT NULL) ON COMMIT DROP");
+    m_db_connection.exec("CREATE TEMP TABLE osm2pgsql_changed_relations"
+                         " (id int8 NOT NULL) ON COMMIT DROP");
+
+    send_id_list(m_db_connection, "osm2pgsql_changed_ways", changed_ways);
+
+    m_db_connection.exec("ANALYZE osm2pgsql_changed_ways");
+
+    if (m_options->middle_database_format == 1) {
+        m_db_connection.exec(build_sql(*m_options, R"(
+INSERT INTO osm2pgsql_changed_relations
+  SELECT DISTINCT r.id
+    FROM {schema}"{prefix}_rels" r, osm2pgsql_changed_ways w
+    WHERE r.parts && ARRAY[w.id]
+      AND r.parts[way_off+1:rel_off] && ARRAY[w.id]
+        )"));
+    } else {
+        m_db_connection.exec(build_sql(*m_options, R"(
+INSERT INTO osm2pgsql_changed_relations
+  SELECT DISTINCT r.id
+    FROM {schema}"{prefix}_rels" r, osm2pgsql_changed_ways c
+    WHERE {schema}"{prefix}_member_ids"(r.members, 'W'::char) && ARRAY[c.id];
+        )"));
+    }
+
+    load_id_list(m_db_connection, "osm2pgsql_changed_relations",
+                 parent_relations);
+
+    m_db_connection.exec("COMMIT");
+
+    timer.stop();
+    log_debug("Found {} ways that are new/changed in input or parent of"
+              " changed node.",
+              changed_ways.size());
+    log_debug("  Found in {} their {} parent relations.",
+              std::chrono::duration_cast<std::chrono::seconds>(timer.elapsed()),
+              parent_relations->size() - num_relations_referenced_by_nodes);
+
+    // (Potentially) contains parent relations from nodes and from ways. Make
+    // sure they are merged.
+    parent_relations->sort_unique();
 }
 
 void middle_pgsql_t::way_set(osmium::Way const &way)
@@ -1223,13 +1269,6 @@ void middle_pgsql_t::start()
         // problems when accessing the intarrays.
         m_db_connection.set_config("jit_above_cost", "-1");
         m_db_connection.set_config("max_parallel_workers_per_gather", "0");
-
-        // Prepare queries for updating dependent objects
-        for (auto &table : m_tables) {
-            if (!table.m_prepare_fw_dep_lookups.empty()) {
-                m_db_connection.exec(table.m_prepare_fw_dep_lookups);
-            }
-        }
     } else {
         if (m_store_options.db_format == 2) {
             table_setup(m_db_connection, m_users_table);
@@ -1488,12 +1527,6 @@ static table_sql sql_for_relations_format1()
                         "  SELECT members, tags"
                         "    FROM {schema}\"{prefix}_rels\" WHERE id = $1;\n";
 
-    sql.prepare_fw_dep_lookups =
-        "PREPARE mark_rels_by_way(int8) AS"
-        "  SELECT id FROM {schema}\"{prefix}_rels\""
-        "    WHERE parts && ARRAY[$1]"
-        "      AND parts[way_off+1:rel_off] && ARRAY[$1];\n";
-
     sql.create_fw_dep_indexes =
         "CREATE INDEX ON {schema}\"{prefix}_rels\" USING GIN (parts)"
         "  WITH (fastupdate = off) {index_tablespace};\n";
@@ -1528,12 +1561,6 @@ static table_sql sql_for_relations_format2()
                         " {users_table_access}"
                         " WHERE o.id = $1;\n";
 
-    sql.prepare_fw_dep_lookups =
-        "PREPARE mark_rels_by_way(int8) AS"
-        "  SELECT DISTINCT id FROM {schema}\"{prefix}_rels\""
-        "    WHERE {schema}\"{prefix}_member_ids\"(members, 'W'::char)"
-        "      @> ARRAY[$1::bigint];\n";
-
     sql.create_fw_dep_indexes =
         "CREATE INDEX \"{prefix}_rels_node_members\""
         "  ON {schema}\"{prefix}_rels\" USING GIN"

src/middle-pgsql.hpp

@@ -95,7 +95,6 @@ struct table_sql
     std::string name;
     std::string create_table;
     std::string prepare_query;
-    std::string prepare_fw_dep_lookups;
     std::string create_fw_dep_indexes;
 };
 
@@ -122,7 +121,9 @@ struct middle_pgsql_t : public middle_t
         osmium::index::IdSetSmall<osmid_t> *parent_ways,
         osmium::index::IdSetSmall<osmid_t> *parent_relations) const override;
 
-    idlist_t get_rels_by_way(osmid_t osm_id) override;
+    void get_way_parents(
+        osmium::index::IdSetSmall<osmid_t> const &changed_ways,
+        osmium::index::IdSetSmall<osmid_t> *parent_relations) const override;
 
     class table_desc
     {
@@ -150,7 +151,6 @@ struct middle_pgsql_t : public middle_t
 
         std::string m_create_table;
         std::string m_prepare_query;
-        std::string m_prepare_fw_dep_lookups;
         std::string m_create_fw_dep_indexes;
 
         void task_set(std::future<std::chrono::microseconds> &&future)

src/middle.hpp

@@ -156,7 +156,11 @@ class middle_t
     {
     }
 
-    virtual idlist_t get_rels_by_way(osmid_t) { return {}; }
+    virtual void get_way_parents(
+        osmium::index::IdSetSmall<osmid_t> const & /*changed_ways*/,
+        osmium::index::IdSetSmall<osmid_t> * /*parent_relations*/) const
+    {
+    }
 
     virtual std::shared_ptr<middle_query_t> get_query_instance() = 0;
 

src/osmdata.cpp

@@ -109,6 +109,9 @@ void osmdata_t::after_ways()
 {
     m_mid->after_ways();
     m_output->after_ways();
+    if (m_append) {
+        m_dependency_manager->after_ways();
+    }
 }
 
 void osmdata_t::relation(osmium::Relation const &rel)
@@ -351,10 +354,13 @@ void osmdata_t::process_dependents() const
     }
 
     // stage 1c processing: mark parent relations of marked objects as changed
-    for (auto const id : m_output->get_marked_way_ids()) {
-        m_dependency_manager->way_changed(id);
+    auto marked_ways = m_output->get_marked_way_ids();
+    if (marked_ways.empty()) {
+        return;
     }
 
+    m_dependency_manager->mark_parent_relations_as_pending(marked_ways);
+
     // process parent relations of marked ways
     if (m_dependency_manager->has_pending()) {
         proc.process_relations_stage1c(

src/pgsql-helper.cpp

@@ -25,13 +25,6 @@ idlist_t get_ids_from_result(pg_result_t const &result) {
     return ids;
 }
 
-idlist_t get_ids_from_db(pg_conn_t const *db_connection, char const *stmt,
-                         osmid_t id)
-{
-    auto const res = db_connection->exec_prepared(stmt, id);
-    return get_ids_from_result(res);
-}
-
 void create_geom_check_trigger(pg_conn_t *db_connection,
                                std::string const &schema,
                                std::string const &table,

src/pgsql-helper.hpp

@@ -27,9 +27,6 @@ class pg_result_t;
  */
 idlist_t get_ids_from_result(pg_result_t const &result);
 
-idlist_t get_ids_from_db(pg_conn_t const *db_connection, char const *stmt,
-                         osmid_t id);
-
 void create_geom_check_trigger(pg_conn_t *db_connection,
                                std::string const &schema,
                                std::string const &table,

tests/test-middle.cpp

@@ -1107,6 +1107,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default,
         mid->after_nodes();
         dependency_manager.after_nodes();
         mid->after_ways();
+        dependency_manager.after_ways();
         mid->after_relations();
 
         REQUIRE(dependency_manager.has_pending());
@@ -1141,6 +1142,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default,
             mid->after_nodes();
             dependency_manager.after_nodes();
             mid->after_ways();
+            dependency_manager.after_ways();
             mid->after_relations();
 
             REQUIRE(dependency_manager.has_pending());
@@ -1181,6 +1183,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default,
             mid->after_nodes();
             dependency_manager.after_nodes();
             mid->after_ways();
+            dependency_manager.after_ways();
             mid->after_relations();
 
             REQUIRE_FALSE(dependency_manager.has_pending());
@@ -1247,6 +1250,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in relation", "", options_slim_default,
         mid->after_nodes();
         dependency_manager.after_nodes();
         mid->after_ways();
+        dependency_manager.after_ways();
         mid->after_relations();
 
         REQUIRE(dependency_manager.has_pending());
@@ -1268,6 +1272,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in relation", "", options_slim_default,
         mid->after_nodes();
         dependency_manager.after_nodes();
         mid->after_ways();
+        dependency_manager.after_ways();
         mid->after_relations();
 
         REQUIRE(dependency_manager.has_pending());