Merge pull request #2244 from joto/robust-properties

lonvia · web-flow · commit fa5b15280371 · 2024-09-16T21:49:30.000+02:00
Make property handling more robust
diff --git a/src/gen/osm2pgsql-gen.cpp b/src/gen/osm2pgsql-gen.cpp
@@ -730,7 +730,11 @@ int main(int argc, char *argv[])
         }
 
         properties_t properties{connection_params, middle_dbschema};
-        properties.load();
+        if (!properties.load()) {
+            throw std::runtime_error{
+                "Did not find table 'osm2pgsql_properties' in database. "
+                "Database too old? Wrong schema?"};
+        }
 
         if (style.empty()) {
             style = properties.get_string("style", "");
diff --git a/src/osm2pgsql.cpp b/src/osm2pgsql.cpp
@@ -44,7 +44,7 @@ void show_memory_usage()
     }
 }
 
-file_info run(options_t const &options, properties_t const &properties)
+file_info run(options_t const &options, properties_t *properties)
 {
     auto const files = prepare_input_files(
         options.input_files, options.input_format, options.append);
@@ -57,10 +57,15 @@ file_info run(options_t const &options, properties_t const &properties)
     middle->start();
 
     auto output = output_t::create_output(middle->get_query_instance(),
-                                          thread_pool, options, properties);
+                                          thread_pool, options, *properties);
 
     middle->set_requirements(output->get_requirements());
 
+    if (!options.append) {
+        properties->init_table();
+    }
+    properties->store();
+
     osmdata_t osmdata{middle, output, options};
 
     // Processing: In this phase the input file(s) are read and parsed,
@@ -93,8 +98,8 @@ void check_db(options_t const &options)
     check_schema(options.output_dbschema);
 }
 
-// This is called in "create" mode to store properties into the database.
-void store_properties(properties_t *properties, options_t const &options)
+// This is called in "create" mode to initialize properties.
+void set_up_properties(properties_t *properties, options_t const &options)
 {
     properties->set_bool("attributes", options.extra_attributes);
 
@@ -121,8 +126,6 @@ void store_properties(properties_t *properties, options_t const &options)
             std::filesystem::absolute(std::filesystem::path{options.style})
                 .string());
     }
-
-    properties->store();
 }
 
 void store_data_properties(properties_t *properties, file_info const &finfo)
@@ -139,8 +142,6 @@ void store_data_properties(properties_t *properties, file_info const &finfo)
             properties->set_string("replication_" + s, value);
         }
     }
-
-    properties->store();
 }
 
 void check_updatable(properties_t const &properties)
@@ -206,7 +207,7 @@ void check_and_update_flat_node_file(properties_t *properties,
                 "Using the flat node file you specified on the command line"
                 " ('{}') instead of the one used on import ('{}').",
                 absolute_path, flat_node_file_from_import);
-            properties->set_string("flat_node_file", absolute_path, true);
+            properties->set_string("flat_node_file", absolute_path);
         }
     }
 }
@@ -291,7 +292,7 @@ void check_and_update_style_file(properties_t *properties, options_t *options)
     log_info("Using the style file you specified on the command line"
              " ('{}') instead of the one used on import ('{}').",
              absolute_path, style_file_from_import);
-    properties->set_string("style", absolute_path, true);
+    properties->set_string("style", absolute_path);
 }
 
 // This is called in "append" mode to check that the command line options are
@@ -348,6 +349,7 @@ int main(int argc, char *argv[])
 
         properties_t properties{options.connection_params,
                                 options.middle_dbschema};
+
         if (options.append) {
             if (!properties.load()) {
                 throw std::runtime_error{
@@ -356,8 +358,9 @@ int main(int argc, char *argv[])
             }
 
             check_and_update_properties(&properties, &options);
+            properties.store();
 
-            auto const finfo = run(options, properties);
+            auto const finfo = run(options, &properties);
 
             if (finfo.last_timestamp.valid()) {
                 auto const current_timestamp =
@@ -367,16 +370,18 @@ int main(int argc, char *argv[])
                     (finfo.last_timestamp >
                      osmium::Timestamp{current_timestamp})) {
                     properties.set_string("current_timestamp",
-                                          finfo.last_timestamp.to_iso(), true);
+                                          finfo.last_timestamp.to_iso());
                 }
             }
         } else {
             set_option_defaults(&options);
-            store_properties(&properties, options);
-            auto const finfo = run(options, properties);
+            set_up_properties(&properties, options);
+            auto const finfo = run(options, &properties);
             store_data_properties(&properties, finfo);
         }
 
+        properties.store();
+
         show_memory_usage();
         log_info("osm2pgsql took {} overall.",
                  util::human_readable_duration(timer_overall.stop()));
diff --git a/src/properties.cpp b/src/properties.cpp
@@ -83,66 +83,55 @@ bool properties_t::get_bool(std::string const &property,
                     property);
 }
 
-void properties_t::set_string(std::string property, std::string value,
-                              bool update_database)
+void properties_t::set_string(std::string property, std::string value)
 {
-    auto const r =
-        m_properties.insert_or_assign(std::move(property), std::move(value));
-
-    if (!update_database || !m_has_properties_table) {
-        return;
-    }
-
-    auto const &inserted = *(r.first);
-    log_debug("  Storing {}='{}'", inserted.first, inserted.second);
+    m_properties[property] = value;
+    m_to_update[property] = value;
+}
 
-    pg_conn_t const db_connection{m_connection_params, "prop.set"};
-    db_connection.exec(
-        "PREPARE set_property(text, text) AS"
-        " INSERT INTO {} (property, value) VALUES ($1, $2)"
-        " ON CONFLICT (property) DO UPDATE SET value = EXCLUDED.value",
-        table_name());
-    db_connection.exec_prepared("set_property", inserted.first,
-                                inserted.second);
+void properties_t::set_int(std::string property, int64_t value)
+{
+    set_string(std::move(property), std::to_string(value));
 }
 
-void properties_t::set_int(std::string property, int64_t value,
-                           bool update_database)
+void properties_t::set_bool(std::string property, bool value)
 {
-    set_string(std::move(property), std::to_string(value), update_database);
+    set_string(std::move(property), value ? "true" : "false");
 }
 
-void properties_t::set_bool(std::string property, bool value,
-                            bool update_database)
+void properties_t::init_table()
 {
-    set_string(std::move(property), value ? "true" : "false", update_database);
+    auto const table = table_name();
+    log_info("Initializing properties table '{}'.", table);
+
+    pg_conn_t const db_connection{m_connection_params, "prop.store"};
+    db_connection.exec("CREATE TABLE IF NOT EXISTS {} ("
+                       " property TEXT NOT NULL PRIMARY KEY,"
+                       " value TEXT NOT NULL)",
+                       table);
+    db_connection.exec("TRUNCATE {}", table);
+    m_has_properties_table = true;
 }
 
 void properties_t::store()
 {
     auto const table = table_name();
-
     log_info("Storing properties to table '{}'.", table);
-    pg_conn_t const db_connection{m_connection_params, "prop.store"};
 
-    if (m_has_properties_table) {
-        db_connection.exec("TRUNCATE {}", table);
-    } else {
-        db_connection.exec("CREATE TABLE {} ("
-                           " property TEXT NOT NULL PRIMARY KEY,"
-                           " value TEXT NOT NULL)",
-                           table);
-        m_has_properties_table = true;
-    }
+    pg_conn_t const db_connection{m_connection_params, "prop.store"};
 
-    db_connection.exec("PREPARE set_property(text, text) AS"
-                       " INSERT INTO {} (property, value) VALUES ($1, $2)",
-                       table);
+    db_connection.exec(
+        "PREPARE set_property(text, text) AS"
+        " INSERT INTO {} (property, value) VALUES ($1, $2)"
+        " ON CONFLICT (property) DO UPDATE SET value = EXCLUDED.value",
+        table);
 
-    for (auto const &[k, v] : m_properties) {
+    for (auto const &[k, v] : m_to_update) {
         log_debug("  Storing {}='{}'", k, v);
         db_connection.exec_prepared("set_property", k, v);
     }
+
+    m_to_update.clear();
 }
 
 bool properties_t::load()
diff --git a/src/properties.hpp b/src/properties.hpp
@@ -53,37 +53,36 @@ class properties_t
      *
      * \param property Name of the property
      * \param value Value of the property
-     * \param update_database Update database with this value immediately.
      */
-    void set_string(std::string property, std::string value,
-                    bool update_database = false);
+    void set_string(std::string property, std::string value);
 
     /**
      * Set property to integer value. The integer will be converted to a string
      * internally.
      *
      * \param property Name of the property
      * \param value Value of the property
-     * \param update_database Update database with this value immediately.
      */
-    void set_int(std::string property, int64_t value,
-                 bool update_database = false);
+    void set_int(std::string property, int64_t value);
 
     /**
      * Set property to boolean value. In the database this will show up as the
      * string 'true' or 'false'.
      *
      * \param property Name of the property
      * \param value Value of the property
-     * \param update_database Update database with this value immediately.
      */
-    void set_bool(std::string property, bool value,
-                  bool update_database = false);
+    void set_bool(std::string property, bool value);
 
     /**
-     * Store all properties in the database. Creates the properties table in
-     * the database if needed. Removes any properties that might already be
-     * stored in the database.
+     * Initialize the database table 'osm2pgsql_properties'. It is created if
+     * it does not exist and truncated.
+     */
+    void init_table();
+
+    /**
+     * Store all properties in the database that changed since the last store.
+     * Overwrites any properties that might already be stored in the database.
      */
     void store();
 
@@ -102,7 +101,13 @@ class properties_t
 private:
     std::string table_name() const;
 
+    // The properties
     std::map<std::string, std::string> m_properties;
+
+    // Temporary storage of all properties that need to be updated in the
+    // database.
+    std::map<std::string, std::string> m_to_update;
+
     connection_params_t m_connection_params;
     std::string m_schema;
     bool m_has_properties_table;
diff --git a/tests/test-properties.cpp b/tests/test-properties.cpp
@@ -46,13 +46,15 @@ TEST_CASE("Store and retrieve properties (with database)")
 {
     for (std::string const schema : {"public", "middleschema"}) {
         testing::pg::tempdb_t const db;
-        auto conn = db.connect();
+        auto const conn = db.connect();
+
         if (schema != "public") {
             conn.exec("CREATE SCHEMA IF NOT EXISTS {};", schema);
         }
 
         {
             properties_t properties{db.connection_params(), schema};
+            properties.init_table();
 
             properties.set_string("foo", "bar");
             properties.set_string("empty", "");
@@ -103,10 +105,11 @@ TEST_CASE("Store and retrieve properties (with database)")
 TEST_CASE("Update existing properties in database")
 {
     testing::pg::tempdb_t const db;
-    auto conn = db.connect();
+    auto const conn = db.connect();
 
     {
         properties_t properties{db.connection_params(), "public"};
+        properties.init_table();
 
         properties.set_string("a", "xxx");
         properties.set_string("b", "yyy");
@@ -124,12 +127,14 @@ TEST_CASE("Update existing properties in database")
         REQUIRE(properties.get_string("a", "def") == "xxx");
         REQUIRE(properties.get_string("b", "def") == "yyy");
 
-        properties.set_string("a", "zzz", false);
-        properties.set_string("b", "zzz", true);
+        properties.set_string("a", "zzz");
+        properties.set_string("b", "zzz");
 
         // both are updated in memory
         REQUIRE(properties.get_string("a", "def") == "zzz");
         REQUIRE(properties.get_string("b", "def") == "zzz");
+
+        properties.store();
     }
 
     {
@@ -138,16 +143,16 @@ TEST_CASE("Update existing properties in database")
         properties_t properties{db.connection_params(), "public"};
         REQUIRE(properties.load());
 
-        // only "b" was updated in the database
-        REQUIRE(properties.get_string("a", "def") == "xxx");
+        // both are updated in the database
+        REQUIRE(properties.get_string("a", "def") == "zzz");
         REQUIRE(properties.get_string("b", "def") == "zzz");
     }
 }
 
 TEST_CASE("Load returns false if there are no properties in database")
 {
     testing::pg::tempdb_t const db;
-    auto conn = db.connect();
+    auto const conn = db.connect();
     init_database_capabilities(conn);
 
     properties_t properties{db.connection_params(), "public"};

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ void show_memory_usage()`
`44`	`44`	`}`
`45`	`45`	`}`
`46`	`46`
`47`		`-file_info run(options_t const &options, properties_t const &properties)`
	`47`	`+file_info run(options_t const &options, properties_t *properties)`
`48`	`48`	`{`
`49`	`49`	`auto const files = prepare_input_files(`
`50`	`50`	`options.input_files, options.input_format, options.append);`
`@@ -57,10 +57,15 @@ file_info run(options_t const &options, properties_t const &properties)`
`57`	`57`	`middle->start();`
`58`	`58`
`59`	`59`	`auto output = output_t::create_output(middle->get_query_instance(),`
`60`		`- thread_pool, options, properties);`
	`60`	`+ thread_pool, options, *properties);`
`61`	`61`
`62`	`62`	`middle->set_requirements(output->get_requirements());`
`63`	`63`
	`64`	`+ if (!options.append) {`
	`65`	`+ properties->init_table();`
	`66`	`+ }`
	`67`	`+ properties->store();`
	`68`	`+`
`64`	`69`	`osmdata_t osmdata{middle, output, options};`
`65`	`70`
`66`	`71`	`// Processing: In this phase the input file(s) are read and parsed,`
`@@ -93,8 +98,8 @@ void check_db(options_t const &options)`
`93`	`98`	`check_schema(options.output_dbschema);`
`94`	`99`	`}`
`95`	`100`
`96`		`-// This is called in "create" mode to store properties into the database.`
`97`		`-void store_properties(properties_t *properties, options_t const &options)`
	`101`	`+// This is called in "create" mode to initialize properties.`
	`102`	`+void set_up_properties(properties_t *properties, options_t const &options)`
`98`	`103`	`{`
`99`	`104`	`properties->set_bool("attributes", options.extra_attributes);`
`100`	`105`
`@@ -121,8 +126,6 @@ void store_properties(properties_t *properties, options_t const &options)`
`121`	`126`	`std::filesystem::absolute(std::filesystem::path{options.style})`
`122`	`127`	`.string());`
`123`	`128`	`}`
`124`		`-`
`125`		`- properties->store();`
`126`	`129`	`}`
`127`	`130`
`128`	`131`	`void store_data_properties(properties_t *properties, file_info const &finfo)`
`@@ -139,8 +142,6 @@ void store_data_properties(properties_t *properties, file_info const &finfo)`
`139`	`142`	`properties->set_string("replication_" + s, value);`
`140`	`143`	`}`
`141`	`144`	`}`
`142`		`-`
`143`		`- properties->store();`
`144`	`145`	`}`
`145`	`146`
`146`	`147`	`void check_updatable(properties_t const &properties)`
`@@ -206,7 +207,7 @@ void check_and_update_flat_node_file(properties_t *properties,`
`206`	`207`	`"Using the flat node file you specified on the command line"`
`207`	`208`	`" ('{}') instead of the one used on import ('{}').",`
`208`	`209`	`absolute_path, flat_node_file_from_import);`
`209`		`- properties->set_string("flat_node_file", absolute_path, true);`
	`210`	`+ properties->set_string("flat_node_file", absolute_path);`
`210`	`211`	`}`
`211`	`212`	`}`
`212`	`213`	`}`
`@@ -291,7 +292,7 @@ void check_and_update_style_file(properties_t properties, options_t options)`
`291`	`292`	`log_info("Using the style file you specified on the command line"`
`292`	`293`	`" ('{}') instead of the one used on import ('{}').",`
`293`	`294`	`absolute_path, style_file_from_import);`
`294`		`- properties->set_string("style", absolute_path, true);`
	`295`	`+ properties->set_string("style", absolute_path);`
`295`	`296`	`}`
`296`	`297`
`297`	`298`	`// This is called in "append" mode to check that the command line options are`
`@@ -348,6 +349,7 @@ int main(int argc, char *argv[])`
`348`	`349`
`349`	`350`	`properties_t properties{options.connection_params,`
`350`	`351`	`options.middle_dbschema};`
	`352`	`+`
`351`	`353`	`if (options.append) {`
`352`	`354`	`if (!properties.load()) {`
`353`	`355`	`throw std::runtime_error{`
`@@ -356,8 +358,9 @@ int main(int argc, char *argv[])`
`356`	`358`	`}`
`357`	`359`
`358`	`360`	`check_and_update_properties(&properties, &options);`
	`361`	`+ properties.store();`
`359`	`362`
`360`		`- auto const finfo = run(options, properties);`
	`363`	`+ auto const finfo = run(options, &properties);`
`361`	`364`
`362`	`365`	`if (finfo.last_timestamp.valid()) {`
`363`	`366`	`auto const current_timestamp =`
`@@ -367,16 +370,18 @@ int main(int argc, char *argv[])`
`367`	`370`	`(finfo.last_timestamp >`
`368`	`371`	`osmium::Timestamp{current_timestamp})) {`
`369`	`372`	`properties.set_string("current_timestamp",`
`370`		`- finfo.last_timestamp.to_iso(), true);`
	`373`	`+ finfo.last_timestamp.to_iso());`
`371`	`374`	`}`
`372`	`375`	`}`
`373`	`376`	`} else {`
`374`	`377`	`set_option_defaults(&options);`
`375`		`- store_properties(&properties, options);`
`376`		`- auto const finfo = run(options, properties);`
	`378`	`+ set_up_properties(&properties, options);`
	`379`	`+ auto const finfo = run(options, &properties);`
`377`	`380`	`store_data_properties(&properties, finfo);`
`378`	`381`	`}`
`379`	`382`
	`383`	`+ properties.store();`
	`384`	`+`
`380`	`385`	`show_memory_usage();`
`381`	`386`	`log_info("osm2pgsql took {} overall.",`
`382`	`387`	`util::human_readable_duration(timer_overall.stop()));`
Original file line number	Diff line number	Diff line change
`@@ -46,13 +46,15 @@ TEST_CASE("Store and retrieve properties (with database)")`
`46`	`46`	`{`
`47`	`47`	`for (std::string const schema : {"public", "middleschema"}) {`
`48`	`48`	`testing::pg::tempdb_t const db;`
`49`		`- auto conn = db.connect();`
	`49`	`+ auto const conn = db.connect();`
	`50`	`+`
`50`	`51`	`if (schema != "public") {`
`51`	`52`	`conn.exec("CREATE SCHEMA IF NOT EXISTS {};", schema);`
`52`	`53`	`}`
`53`	`54`
`54`	`55`	`{`
`55`	`56`	`properties_t properties{db.connection_params(), schema};`
	`57`	`+ properties.init_table();`
`56`	`58`
`57`	`59`	`properties.set_string("foo", "bar");`
`58`	`60`	`properties.set_string("empty", "");`
`@@ -103,10 +105,11 @@ TEST_CASE("Store and retrieve properties (with database)")`
`103`	`105`	`TEST_CASE("Update existing properties in database")`
`104`	`106`	`{`
`105`	`107`	`testing::pg::tempdb_t const db;`
`106`		`- auto conn = db.connect();`
	`108`	`+ auto const conn = db.connect();`
`107`	`109`
`108`	`110`	`{`
`109`	`111`	`properties_t properties{db.connection_params(), "public"};`
	`112`	`+ properties.init_table();`
`110`	`113`
`111`	`114`	`properties.set_string("a", "xxx");`
`112`	`115`	`properties.set_string("b", "yyy");`
`@@ -124,12 +127,14 @@ TEST_CASE("Update existing properties in database")`
`124`	`127`	`REQUIRE(properties.get_string("a", "def") == "xxx");`
`125`	`128`	`REQUIRE(properties.get_string("b", "def") == "yyy");`
`126`	`129`
`127`		`- properties.set_string("a", "zzz", false);`
`128`		`- properties.set_string("b", "zzz", true);`
	`130`	`+ properties.set_string("a", "zzz");`
	`131`	`+ properties.set_string("b", "zzz");`
`129`	`132`
`130`	`133`	`// both are updated in memory`
`131`	`134`	`REQUIRE(properties.get_string("a", "def") == "zzz");`
`132`	`135`	`REQUIRE(properties.get_string("b", "def") == "zzz");`
	`136`	`+`
	`137`	`+ properties.store();`
`133`	`138`	`}`
`134`	`139`
`135`	`140`	`{`
`@@ -138,16 +143,16 @@ TEST_CASE("Update existing properties in database")`
`138`	`143`	`properties_t properties{db.connection_params(), "public"};`
`139`	`144`	`REQUIRE(properties.load());`
`140`	`145`
`141`		`- // only "b" was updated in the database`
`142`		`- REQUIRE(properties.get_string("a", "def") == "xxx");`
	`146`	`+ // both are updated in the database`
	`147`	`+ REQUIRE(properties.get_string("a", "def") == "zzz");`
`143`	`148`	`REQUIRE(properties.get_string("b", "def") == "zzz");`
`144`	`149`	`}`
`145`	`150`	`}`
`146`	`151`
`147`	`152`	`TEST_CASE("Load returns false if there are no properties in database")`
`148`	`153`	`{`
`149`	`154`	`testing::pg::tempdb_t const db;`
`150`		`- auto conn = db.connect();`
	`155`	`+ auto const conn = db.connect();`
`151`	`156`	`init_database_capabilities(conn);`
`152`	`157`
`153`	`158`	`properties_t properties{db.connection_params(), "public"};`