Skip to content

Commit

Permalink
Merge pull request #2244 from joto/robust-properties
Browse files Browse the repository at this point in the history 
Make property handling more robust
  • Loading branch information
@lonvia
lonvia authored Sep 16, 2024
2 parents 0b3271e + 7b77762 commit fa5b152
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 74 deletions.
6 changes: 5 additions & 1 deletion src/gen/osm2pgsql-gen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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", "");
Expand Down
33 changes: 19 additions & 14 deletions src/osm2pgsql.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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,
Expand Down Expand Up @@ -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);

Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand All @@ -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 =
Expand All @@ -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()));
Expand Down
69 changes: 29 additions & 40 deletions src/properties.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
29 changes: 17 additions & 12 deletions src/properties.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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;
Expand Down
19 changes: 12 additions & 7 deletions tests/test-properties.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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", "");
Expand Down Expand Up @@ -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");
Expand All @@ -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();
}

{
Expand All @@ -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"};
Expand Down

0 comments on commit fa5b152

Please sign in to comment.