From 0592491934cde42ba5b79186efaee0ec6e033272 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Mon, 6 Apr 2020 15:49:50 +0200 Subject: [PATCH 1/9] Clean up middle test code * Change order of add_node() parameters to match Location * Add add_node_and_get() and add_way_and_get() helpers * Add all vector constructors to idlist_t --- src/osmtypes.hpp | 7 ++++++- tests/test-middle.cpp | 37 ++++++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/osmtypes.hpp b/src/osmtypes.hpp index a2dbed931..a461aa406 100644 --- a/src/osmtypes.hpp +++ b/src/osmtypes.hpp @@ -232,7 +232,12 @@ class taglist_t struct idlist_t : public std::vector { - idlist_t() {} + // Get all constructors from std::vector + using vector::vector; + + // Even though we got all constructors from std::vector we need this on + // some compilers/libraries for some reason. + idlist_t() = default; explicit idlist_t(osmium::NodeRefList const &list) { diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index d39cc8d3e..5c2f526c4 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -16,13 +16,13 @@ namespace { /// Simple osmium buffer to store object with some convenience. struct test_buffer_t { - size_t add_node(osmid_t id, double lat, double lon) + size_t add_node(osmid_t id, double lon, double lat) { using namespace osmium::builder::attr; return osmium::builder::add_node(buf, _id(id), _location(lon, lat)); } - size_t add_way(osmid_t wid, std::vector const &ids) + size_t add_way(osmid_t wid, idlist_t const &ids) { using namespace osmium::builder::attr; return osmium::builder::add_way(buf, _id(wid), _nodes(ids)); @@ -40,6 +40,16 @@ struct test_buffer_t return buf.get(pos); } + osmium::Node const &add_node_and_get(osmid_t id, double lon, double lat) + { + return get(add_node(id, lon, lat)); + } + + osmium::Way &add_way_and_get(osmid_t wid, idlist_t const &ids) + { + return get(add_way(wid, ids)); + } + osmium::memory::Buffer buf{4096, osmium::memory::Buffer::auto_grow::yes}; }; @@ -125,32 +135,30 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, SECTION("Set and retrieve a single node") { - auto const &node = buffer.get( - buffer.add_node(1234, 12.3456789, 98.7654321)); + auto const &node = + buffer.add_node_and_get(1234, 98.7654321, 12.3456789); // set the node mid->nodes_set(node); mid->flush(); // getting it back works only via a waylist - auto &nodes = - buffer.get(buffer.add_way(3, {1234})).nodes(); + auto &nodes = buffer.add_way_and_get(3, {1234}).nodes(); // get it back REQUIRE(mid_q->nodes_get_list(&nodes) == nodes.size()); expect_location(nodes[0].location(), node); // other nodes are not retrievable - auto &n2 = - buffer.get(buffer.add_way(3, {1, 2, 1235})).nodes(); + auto &n2 = buffer.add_way_and_get(3, {1, 2, 1235}).nodes(); REQUIRE(mid_q->nodes_get_list(&n2) == 0); } SECTION("Set and retrieve a single way") { osmid_t const way_id = 1; - double const lat = 12.3456789; double const lon = 98.7654321; + double const lat = 12.3456789; idlist_t nds; std::vector nodes; @@ -158,12 +166,12 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, for (osmid_t i = 1; i <= 10; ++i) { nds.push_back(i); nodes.push_back( - buffer.add_node(i, lat + i * 0.001, lon - i * 0.003)); + buffer.add_node(i, lon - i * 0.003, lat + i * 0.001)); mid->nodes_set(buffer.get(nodes.back())); } // set the way - mid->ways_set(buffer.get(buffer.add_way(way_id, nds))); + mid->ways_set(buffer.add_way_and_get(way_id, nds)); mid->flush(); @@ -189,16 +197,15 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, SECTION("Set and retrieve a single relation with supporting ways") { - std::vector const nds[] = { - {4, 5, 13, 14, 342}, {45, 90}, {30, 3, 45}}; + idlist_t const nds[] = {{4, 5, 13, 14, 342}, {45, 90}, {30, 3, 45}}; // set the node - mid->nodes_set(buffer.get(buffer.add_node(1, 12.8, 4.1))); + mid->nodes_set(buffer.add_node_and_get(1, 4.1, 12.8)); // set the ways osmid_t wid = 10; for (auto const &n : nds) { - mid->ways_set(buffer.get(buffer.add_way(wid, n))); + mid->ways_set(buffer.add_way_and_get(wid, n)); ++wid; } From 3a618510c305e3be611b42cc3ca599c6b086e346 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Mon, 6 Apr 2020 16:04:32 +0200 Subject: [PATCH 2/9] Add test for adding/deleting/changing way in middle --- tests/test-middle.cpp | 115 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1 deletion(-) diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index 5c2f526c4..c12eca952 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -6,6 +6,7 @@ #include "middle-pgsql.hpp" #include "middle-ram.hpp" +#include "common-cleanup.hpp" #include "common-options.hpp" #include "common-pg.hpp" @@ -68,7 +69,7 @@ struct options_slim_default } }; -struct options_slim_dense_cache : options_slim_default +struct options_slim_dense_cache { static options_t options(pg::tempdb_t const &tmpdb) { @@ -78,6 +79,14 @@ struct options_slim_dense_cache : options_slim_default } }; +struct options_flat_node_cache +{ + static options_t options(pg::tempdb_t const &tmpdb) + { + return testing::opt_t().slim(tmpdb).flatnodes(); + } +}; + struct options_ram_optimized { static options_t options(pg::tempdb_t const &) @@ -258,3 +267,107 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, REQUIRE_FALSE(mid_q->relations_get(999, buffer.buf)); } } + +TEMPLATE_TEST_CASE("middle: add, delete and update way", "", + options_slim_default, options_slim_dense_cache, + options_flat_node_cache) +{ + options_t options = TestType::options(db); + + testing::cleanup::file_t flatnode_cleaner{ + options.flat_node_file.get_value_or("")}; + + osmid_t const way_id = 20; + + // create some nodes we'll use for the ways + test_buffer_t buffer; + auto const& node10 = buffer.add_node_and_get(10, 1.0, 0.0); + auto const& node11 = buffer.add_node_and_get(11, 1.1, 0.0); + auto const& node12 = buffer.add_node_and_get(12, 1.2, 0.0); + + // Set up middle in "create" mode to get a cleanly initialized database + // and add some nodes. + { + auto mid = std::make_shared(&options); + mid->start(); + + mid->nodes_set(node10); + mid->nodes_set(node11); + mid->nodes_set(node12); + mid->flush(); + + // create way + idlist_t nds{10, 11}; + mid->ways_set(buffer.add_way_and_get(way_id, nds)); + mid->flush(); + + osmium::memory::Buffer outbuf{4096, + osmium::memory::Buffer::auto_grow::yes}; + + auto const mid_q = mid->get_query_instance(); + REQUIRE(mid_q->ways_get(way_id, outbuf)); + + auto &way = outbuf.get(0); + + REQUIRE(way.id() == way_id); + REQUIRE(way.nodes().size() == nds.size()); + REQUIRE(way.nodes()[0].ref() == 10); + REQUIRE(way.nodes()[1].ref() == 11); + + REQUIRE(mid_q->nodes_get_list(&(way.nodes())) == nds.size()); + REQUIRE(way.nodes()[0].location() == osmium::Location{1.0, 0.0}); + REQUIRE(way.nodes()[1].location() == osmium::Location{1.1, 0.0}); + + mid->commit(); + } + + // Set up middle again, this time in "append" mode so we can change things. + { + options.append = true; + auto mid = std::make_shared(&options); + mid->start(); + + // delete way + mid->ways_delete(way_id); + mid->flush(); + + osmium::memory::Buffer outbuf{4096, + osmium::memory::Buffer::auto_grow::yes}; + + auto const mid_q = mid->get_query_instance(); + REQUIRE_FALSE(mid_q->ways_get(way_id, outbuf)); + + mid->commit(); + } + + // Set up middle again, still in "append" mode. + { + auto mid = std::make_shared(&options); + mid->start(); + + idlist_t nds{11, 12}; + + // create new version of the way + mid->ways_set(buffer.add_way_and_get(way_id, nds)); + mid->flush(); + + osmium::memory::Buffer outbuf{4096, + osmium::memory::Buffer::auto_grow::yes}; + + auto const mid_q = mid->get_query_instance(); + REQUIRE(mid_q->ways_get(way_id, outbuf)); + + auto &way = outbuf.get(0); + + REQUIRE(way.id() == way_id); + REQUIRE(way.nodes().size() == nds.size()); + REQUIRE(way.nodes()[0].ref() == 11); + REQUIRE(way.nodes()[1].ref() == 12); + + REQUIRE(mid_q->nodes_get_list(&(way.nodes())) == nds.size()); + REQUIRE(way.nodes()[0].location() == osmium::Location{1.1, 0.0}); + REQUIRE(way.nodes()[1].location() == osmium::Location{1.2, 0.0}); + + mid->commit(); + } +} From 6a5ee88cf279c25871697d5edc581416b6ac5520 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 7 Apr 2020 15:06:54 +0200 Subject: [PATCH 3/9] Clean up flat node file after middle tests --- tests/common-cleanup.hpp | 12 +++++++++--- tests/test-middle.cpp | 2 ++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/common-cleanup.hpp b/tests/common-cleanup.hpp index 2c4f5a2c3..c17aa7ac9 100644 --- a/tests/common-cleanup.hpp +++ b/tests/common-cleanup.hpp @@ -1,6 +1,8 @@ #ifndef OSM2PGSQL_TESTS_COMMON_CLEANUP_HPP #define OSM2PGSQL_TESTS_COMMON_CLEANUP_HPP +#include "format.hpp" + #include #include @@ -28,13 +30,17 @@ class file_t ~file_t() noexcept { delete_file(true); } private: - void delete_file(bool warn) noexcept + void delete_file(bool warn) const noexcept { + if (m_filename.empty()) { + return; + } + boost::system::error_code ec; boost::filesystem::remove(m_filename, ec); if (ec && warn) { - fprintf(stderr, "WARNING: Unable to remove \"%s\": %s\n", - m_filename.c_str(), ec.message().c_str()); + fmt::print(stderr, "WARNING: Unable to remove \"{}\": {}\n", + m_filename, ec.message()); } } diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index c12eca952..0a6449282 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -131,6 +131,8 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, options_ram_flatnode) { options_t const options = TestType::options(db); + testing::cleanup::file_t flatnode_cleaner{ + options.flat_node_file.get_value_or("")}; auto mid = options.slim ? std::shared_ptr(new middle_pgsql_t{&options}) From 2464d283282e8fc834fd23aa175285f61fb4b4c4 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 7 Apr 2020 15:22:54 +0200 Subject: [PATCH 4/9] Add default constructor for node_ram_cache for testing --- src/node-ram-cache.hpp | 10 ++++++++-- tests/test-persistent-cache.cpp | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/node-ram-cache.hpp b/src/node-ram-cache.hpp index 8bdde9325..23b444a08 100644 --- a/src/node-ram-cache.hpp +++ b/src/node-ram-cache.hpp @@ -48,6 +48,12 @@ class node_ram_cache }; public: + /** + * The default constructor creates a "dummy" cache which will never + * cache anything. It is used for testing. + */ + node_ram_cache() = default; + node_ram_cache(int strategy, int cacheSizeMB); node_ram_cache(node_ram_cache const &) = delete; @@ -96,7 +102,7 @@ class node_ram_cache osmium::Location get_sparse(osmid_t id) const; osmium::Location get_dense(osmid_t id) const; - int allocStrategy; + int allocStrategy = 0; ramNodeBlock *blocks = nullptr; int usedBlocks = 0; @@ -113,7 +119,7 @@ class node_ram_cache osmid_t maxSparseId = 0; int64_t cacheUsed = 0; - int64_t cacheSize; + int64_t cacheSize = 0; osmid_t storedNodes = 0; osmid_t totalNodes = 0; long nodesCacheHits = 0; diff --git a/tests/test-persistent-cache.cpp b/tests/test-persistent-cache.cpp index 8a16a6078..3c623ffea 100644 --- a/tests/test-persistent-cache.cpp +++ b/tests/test-persistent-cache.cpp @@ -29,7 +29,7 @@ TEST_CASE("Persistent cache", "[NoDB]") { options_t const options = testing::opt_t().flatnodes(); testing::cleanup::file_t flatnode_cleaner{options.flat_node_file.get()}; - auto ram_cache = std::make_shared(0, 0); // empty cache + auto ram_cache = std::make_shared(); // dummy cache // create a new cache { From 4b05337d3b8e674a57d0baa0c4cdc7af250132de Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 7 Apr 2020 15:24:06 +0200 Subject: [PATCH 5/9] read_location() helper only needs a const reference to the cache --- tests/test-persistent-cache.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test-persistent-cache.cpp b/tests/test-persistent-cache.cpp index 3c623ffea..17d75de56 100644 --- a/tests/test-persistent-cache.cpp +++ b/tests/test-persistent-cache.cpp @@ -13,8 +13,8 @@ static void write_and_read_location(node_persistent_cache &cache, osmid_t id, REQUIRE(osmium::Location(x, y) == cache.get(id)); } -static void read_location(node_persistent_cache &cache, osmid_t id, double x, - double y) +static void read_location(node_persistent_cache const &cache, osmid_t id, + double x, double y) { REQUIRE(osmium::Location(x, y) == cache.get(id)); } From c88ca785a392ae0dd3080fbb5ef8689a0e4b9e14 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 7 Apr 2020 15:54:16 +0200 Subject: [PATCH 6/9] Simplify test code --- tests/test-middle.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index 0a6449282..236310b0a 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -226,10 +226,9 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, using namespace osmium::builder::attr; using otype = osmium::item_type; osmium::builder::add_relation( - buffer.buf, _id(123), _member(_member(otype::way, 11)), - _member(_member(otype::way, 10, "outer")), - _member(_member(otype::node, 1)), - _member(_member(otype::way, 12, "inner"))); + buffer.buf, _id(123), _member(otype::way, 11), + _member(otype::way, 10, "outer"), _member(otype::node, 1), + _member(otype::way, 12, "inner")); } osmium::CRC orig_crc; orig_crc.update(buffer.get(pos)); From ddcb625b453784af15ebdd2e1103a5d4fdfe458c Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 7 Apr 2020 16:49:10 +0200 Subject: [PATCH 7/9] Refactor test_buffer_t in middle test New add_relation() function to make adding relations easier. This allows use to make the buffer internal to the class for better encapsulation. --- tests/test-middle.cpp | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index 236310b0a..2cbf8ee28 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -15,8 +15,9 @@ static pg::tempdb_t db; namespace { /// Simple osmium buffer to store object with some convenience. -struct test_buffer_t +class test_buffer_t { +public: size_t add_node(osmid_t id, double lon, double lat) { using namespace osmium::builder::attr; @@ -29,6 +30,15 @@ struct test_buffer_t return osmium::builder::add_way(buf, _id(wid), _nodes(ids)); } + size_t add_relation( + osmid_t rid, + std::initializer_list members) + { + using namespace osmium::builder::attr; + return osmium::builder::add_relation( + buf, _id(rid), _members(members.begin(), members.end())); + } + template T const &get(size_t pos) const { @@ -51,6 +61,7 @@ struct test_buffer_t return get(add_way(wid, ids)); } +private: osmium::memory::Buffer buf{4096, osmium::memory::Buffer::auto_grow::yes}; }; @@ -221,15 +232,11 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, } // set the relation - auto const pos = buffer.buf.committed(); - { - using namespace osmium::builder::attr; - using otype = osmium::item_type; - osmium::builder::add_relation( - buffer.buf, _id(123), _member(otype::way, 11), - _member(otype::way, 10, "outer"), _member(otype::node, 1), - _member(otype::way, 12, "inner")); - } + using otype = osmium::item_type; + auto const pos = buffer.add_relation(123, {{otype::way, 11, ""}, + {otype::way, 10, "outer"}, + {otype::node, 1}, + {otype::way, 12, "inner"}}); osmium::CRC orig_crc; orig_crc.update(buffer.get(pos)); @@ -238,9 +245,10 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, mid->flush(); // retrieve the relation - buffer.buf.clear(); - auto const &rel = buffer.get(0); - REQUIRE(mid_q->relations_get(123, buffer.buf)); + osmium::memory::Buffer outbuf{4096, + osmium::memory::Buffer::auto_grow::yes}; + REQUIRE(mid_q->relations_get(123, outbuf)); + auto const &rel = outbuf.get(0); CHECK(rel.id() == 123); CHECK(rel.members().size() == 4); @@ -251,10 +259,10 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, // retrive the supporting ways rolelist_t roles; - REQUIRE(mid_q->rel_way_members_get(rel, &roles, buffer.buf) == 3); + REQUIRE(mid_q->rel_way_members_get(rel, &roles, outbuf) == 3); REQUIRE(roles.size() == 3); - for (auto &w : buffer.buf.select()) { + for (auto &w : outbuf.select()) { REQUIRE(w.id() >= 10); REQUIRE(w.id() <= 12); auto const &expected = nds[w.id() - 10]; @@ -265,7 +273,7 @@ TEMPLATE_TEST_CASE("middle import", "", options_slim_default, } // other relations are not retrievable - REQUIRE_FALSE(mid_q->relations_get(999, buffer.buf)); + REQUIRE_FALSE(mid_q->relations_get(999, outbuf)); } } From 03eeff9da61e2855fabfcf98f1e9cdbea43c221a Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 7 Apr 2020 17:54:13 +0200 Subject: [PATCH 8/9] Add asserts() in middle-pgsql to expose assumptions the code makes --- src/middle-pgsql.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/middle-pgsql.cpp b/src/middle-pgsql.cpp index 1b468f3c0..f1e35798a 100644 --- a/src/middle-pgsql.cpp +++ b/src/middle-pgsql.cpp @@ -163,6 +163,7 @@ pg_result_t middle_query_pgsql_t::exec_prepared(char const *stmt, pg_result_t middle_pgsql_t::exec_prepared(char const *stmt, osmid_t osm_id) const { + assert(m_query_conn); util::integer_to_buffer buffer{osm_id}; char const *const bptr = buffer.c_str(); return m_query_conn->exec_prepared(stmt, 1, &bptr); @@ -393,6 +394,8 @@ size_t middle_query_pgsql_t::nodes_get_list(osmium::WayNodeList *nodes) const void middle_pgsql_t::nodes_delete(osmid_t osm_id) { + assert(m_append); + if (m_out_options->flat_node_cache_enabled) { m_persistent_cache->set(osm_id, osmium::Location{}); } else { @@ -403,6 +406,7 @@ void middle_pgsql_t::nodes_delete(osmid_t osm_id) void middle_pgsql_t::node_changed(osmid_t osm_id) { + assert(m_append); if (!m_mark_pending) { return; } @@ -519,6 +523,7 @@ middle_query_pgsql_t::rel_way_members_get(osmium::Relation const &rel, void middle_pgsql_t::ways_delete(osmid_t osm_id) { + assert(m_append); m_db_copy.new_line(m_tables[WAY_TABLE].m_copy_target); m_db_copy.delete_object(osm_id); } @@ -539,6 +544,7 @@ void middle_pgsql_t::iterate_ways(middle_t::pending_processor &pf) void middle_pgsql_t::way_changed(osmid_t osm_id) { + assert(m_append); //keep track of whatever rels this way intersects auto const res = exec_prepared("mark_rels_by_way", osm_id); for (int i = 0; i < res.num_tuples(); ++i) { @@ -615,6 +621,7 @@ bool middle_query_pgsql_t::relations_get(osmid_t id, void middle_pgsql_t::relations_delete(osmid_t osm_id) { + assert(m_append); //keep track of whatever ways this relation interesects auto const res = exec_prepared("mark_ways_by_rel", osm_id); for (int i = 0; i < res.num_tuples(); ++i) { @@ -642,6 +649,7 @@ void middle_pgsql_t::iterate_relations(pending_processor &pf) void middle_pgsql_t::relation_changed(osmid_t osm_id) { + assert(m_append); //keep track of whatever ways and rels these nodes intersect //TODO: dont need to stop the copy above since we are only reading? //TODO: can we just mark the id without querying? the where clause seems intersect reltable.parts with the id @@ -667,7 +675,8 @@ idlist_t middle_query_pgsql_t::relations_using_way(osmid_t way_id) const void middle_pgsql_t::analyze() { - for (auto &table : m_tables) { + assert(m_query_conn); + for (auto const &table : m_tables) { m_query_conn->exec("ANALYZE {}"_format(table.name())); } } From 8d7959d29f839ab23ba4a0322c9b0e6ef0521bf7 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Fri, 10 Apr 2020 09:36:47 +0200 Subject: [PATCH 9/9] Cleanup error handling in node ram cache --- src/node-ram-cache.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/node-ram-cache.cpp b/src/node-ram-cache.cpp index b582b2ebb..a11a11d72 100644 --- a/src/node-ram-cache.cpp +++ b/src/node-ram-cache.cpp @@ -90,15 +90,12 @@ void node_ram_cache::set_sparse(osmid_t id, osmium::Location location) return; } else { if (maxSparseId && id < maxSparseId) { - fprintf(stderr, - "Node ids are out of order. Please use slim mode.\n"); - } else { - fprintf( - stderr, - "\nNode cache size is too small to fit all nodes. Please " - "increase cache size\n"); + throw std::runtime_error{ + "Node ids are out of order. Please use slim mode."}; } - throw std::runtime_error{"Using RAM cache."}; + throw std::runtime_error{ + "Node cache size is too small to fit all nodes. Please " + "increase cache size\n"}; } } maxSparseId = id;