Skip to content

Commit 02673cd

Browse files
jotojoto
committed
Simplify property handling and do updates all at once
Property changes were reflected piece by piece in the database. This changes it so they are only done in one go after all the changes have been made. So if something fails before we get to the point where we write all properties to the database, no updates are actually done. And the code is simpler because we keep track of necessary database updates automatically and do them the next time the store() function is called.
1 parent 0b3271e commit 02673cd

File tree

4 files changed

+64
-62
lines changed

4 files changed

+64
-62
lines changed

‎src/osm2pgsql.cpp‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ void check_and_update_flat_node_file(properties_t *properties,
206206
"Using the flat node file you specified on the command line"
207207
" ('{}') instead of the one used on import ('{}').",
208208
absolute_path, flat_node_file_from_import);
209-
properties->set_string("flat_node_file", absolute_path, true);
209+
properties->set_string("flat_node_file", absolute_path);
210210
}
211211
}
212212
}
@@ -291,7 +291,7 @@ void check_and_update_style_file(properties_t *properties, options_t *options)
291291
log_info("Using the style file you specified on the command line"
292292
" ('{}') instead of the one used on import ('{}').",
293293
absolute_path, style_file_from_import);
294-
properties->set_string("style", absolute_path, true);
294+
properties->set_string("style", absolute_path);
295295
}
296296

297297
// This is called in "append" mode to check that the command line options are
@@ -356,6 +356,7 @@ int main(int argc, char *argv[])
356356
}
357357

358358
check_and_update_properties(&properties, &options);
359+
properties.store();
359360

360361
auto const finfo = run(options, properties);
361362

@@ -367,10 +368,12 @@ int main(int argc, char *argv[])
367368
(finfo.last_timestamp >
368369
osmium::Timestamp{current_timestamp})) {
369370
properties.set_string("current_timestamp",
370-
finfo.last_timestamp.to_iso(), true);
371+
finfo.last_timestamp.to_iso());
372+
properties.store();
371373
}
372374
}
373375
} else {
376+
properties.init_table();
374377
set_option_defaults(&options);
375378
store_properties(&properties, options);
376379
auto const finfo = run(options, properties);

‎src/properties.cpp‎

Lines changed: 29 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -83,66 +83,55 @@ bool properties_t::get_bool(std::string const &property,
8383
property);
8484
}
8585

86-
void properties_t::set_string(std::string property, std::string value,
87-
bool update_database)
86+
void properties_t::set_string(std::string property, std::string value)
8887
{
89-
auto const r =
90-
m_properties.insert_or_assign(std::move(property), std::move(value));
91-
92-
if (!update_database || !m_has_properties_table) {
93-
return;
94-
}
95-
96-
auto const &inserted = *(r.first);
97-
log_debug(" Storing {}='{}'", inserted.first, inserted.second);
88+
m_properties[property] = value;
89+
m_to_update[property] = value;
90+
}
9891

99-
pg_conn_t const db_connection{m_connection_params, "prop.set"};
100-
db_connection.exec(
101-
"PREPARE set_property(text, text) AS"
102-
" INSERT INTO {} (property, value) VALUES ($1, $2)"
103-
" ON CONFLICT (property) DO UPDATE SET value = EXCLUDED.value",
104-
table_name());
105-
db_connection.exec_prepared("set_property", inserted.first,
106-
inserted.second);
92+
void properties_t::set_int(std::string property, int64_t value)
93+
{
94+
set_string(std::move(property), std::to_string(value));
10795
}
10896

109-
void properties_t::set_int(std::string property, int64_t value,
110-
bool update_database)
97+
void properties_t::set_bool(std::string property, bool value)
11198
{
112-
set_string(std::move(property), std::to_string(value), update_database);
99+
set_string(std::move(property), value ? "true" : "false");
113100
}
114101

115-
void properties_t::set_bool(std::string property, bool value,
116-
bool update_database)
102+
void properties_t::init_table()
117103
{
118-
set_string(std::move(property), value ? "true" : "false", update_database);
104+
auto const table = table_name();
105+
log_info("Initializing properties table '{}'.", table);
106+
107+
pg_conn_t const db_connection{m_connection_params, "prop.store"};
108+
db_connection.exec("CREATE TABLE IF NOT EXISTS {} ("
109+
" property TEXT NOT NULL PRIMARY KEY,"
110+
" value TEXT NOT NULL)",
111+
table);
112+
db_connection.exec("TRUNCATE {}", table);
113+
m_has_properties_table = true;
119114
}
120115

121116
void properties_t::store()
122117
{
123118
auto const table = table_name();
124-
125119
log_info("Storing properties to table '{}'.", table);
126-
pg_conn_t const db_connection{m_connection_params, "prop.store"};
127120

128-
if (m_has_properties_table) {
129-
db_connection.exec("TRUNCATE {}", table);
130-
} else {
131-
db_connection.exec("CREATE TABLE {} ("
132-
" property TEXT NOT NULL PRIMARY KEY,"
133-
" value TEXT NOT NULL)",
134-
table);
135-
m_has_properties_table = true;
136-
}
121+
pg_conn_t const db_connection{m_connection_params, "prop.store"};
137122

138-
db_connection.exec("PREPARE set_property(text, text) AS"
139-
" INSERT INTO {} (property, value) VALUES ($1, $2)",
140-
table);
123+
db_connection.exec(
124+
"PREPARE set_property(text, text) AS"
125+
" INSERT INTO {} (property, value) VALUES ($1, $2)"
126+
" ON CONFLICT (property) DO UPDATE SET value = EXCLUDED.value",
127+
table);
141128

142-
for (auto const &[k, v] : m_properties) {
129+
for (auto const &[k, v] : m_to_update) {
143130
log_debug(" Storing {}='{}'", k, v);
144131
db_connection.exec_prepared("set_property", k, v);
145132
}
133+
134+
m_to_update.clear();
146135
}
147136

148137
bool properties_t::load()

‎src/properties.hpp‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -53,37 +53,36 @@ class properties_t
5353
*
5454
* \param property Name of the property
5555
* \param value Value of the property
56-
* \param update_database Update database with this value immediately.
5756
*/
58-
void set_string(std::string property, std::string value,
59-
bool update_database = false);
57+
void set_string(std::string property, std::string value);
6058

6159
/**
6260
* Set property to integer value. The integer will be converted to a string
6361
* internally.
6462
*
6563
* \param property Name of the property
6664
* \param value Value of the property
67-
* \param update_database Update database with this value immediately.
6865
*/
69-
void set_int(std::string property, int64_t value,
70-
bool update_database = false);
66+
void set_int(std::string property, int64_t value);
7167

7268
/**
7369
* Set property to boolean value. In the database this will show up as the
7470
* string 'true' or 'false'.
7571
*
7672
* \param property Name of the property
7773
* \param value Value of the property
78-
* \param update_database Update database with this value immediately.
7974
*/
80-
void set_bool(std::string property, bool value,
81-
bool update_database = false);
75+
void set_bool(std::string property, bool value);
8276

8377
/**
84-
* Store all properties in the database. Creates the properties table in
85-
* the database if needed. Removes any properties that might already be
86-
* stored in the database.
78+
* Initialize the database table 'osm2pgsql_properties'. It is created if
79+
* it does not exist and truncated.
80+
*/
81+
void init_table();
82+
83+
/**
84+
* Store all properties in the database that changed since the last store.
85+
* Overwrites any properties that might already be stored in the database.
8786
*/
8887
void store();
8988

@@ -102,7 +101,13 @@ class properties_t
102101
private:
103102
std::string table_name() const;
104103

104+
// The properties
105105
std::map<std::string, std::string> m_properties;
106+
107+
// Temporary storage of all properties that need to be updated in the
108+
// database.
109+
std::map<std::string, std::string> m_to_update;
110+
106111
connection_params_t m_connection_params;
107112
std::string m_schema;
108113
bool m_has_properties_table;

‎tests/test-properties.cpp‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,15 @@ TEST_CASE("Store and retrieve properties (with database)")
4646
{
4747
for (std::string const schema : {"public", "middleschema"}) {
4848
testing::pg::tempdb_t const db;
49-
auto conn = db.connect();
49+
auto const conn = db.connect();
50+
5051
if (schema != "public") {
5152
conn.exec("CREATE SCHEMA IF NOT EXISTS {};", schema);
5253
}
5354

5455
{
5556
properties_t properties{db.connection_params(), schema};
57+
properties.init_table();
5658

5759
properties.set_string("foo", "bar");
5860
properties.set_string("empty", "");
@@ -103,10 +105,11 @@ TEST_CASE("Store and retrieve properties (with database)")
103105
TEST_CASE("Update existing properties in database")
104106
{
105107
testing::pg::tempdb_t const db;
106-
auto conn = db.connect();
108+
auto const conn = db.connect();
107109

108110
{
109111
properties_t properties{db.connection_params(), "public"};
112+
properties.init_table();
110113

111114
properties.set_string("a", "xxx");
112115
properties.set_string("b", "yyy");
@@ -124,12 +127,14 @@ TEST_CASE("Update existing properties in database")
124127
REQUIRE(properties.get_string("a", "def") == "xxx");
125128
REQUIRE(properties.get_string("b", "def") == "yyy");
126129

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");
129132

130133
// both are updated in memory
131134
REQUIRE(properties.get_string("a", "def") == "zzz");
132135
REQUIRE(properties.get_string("b", "def") == "zzz");
136+
137+
properties.store();
133138
}
134139

135140
{
@@ -138,16 +143,16 @@ TEST_CASE("Update existing properties in database")
138143
properties_t properties{db.connection_params(), "public"};
139144
REQUIRE(properties.load());
140145

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");
143148
REQUIRE(properties.get_string("b", "def") == "zzz");
144149
}
145150
}
146151

147152
TEST_CASE("Load returns false if there are no properties in database")
148153
{
149154
testing::pg::tempdb_t const db;
150-
auto conn = db.connect();
155+
auto const conn = db.connect();
151156
init_database_capabilities(conn);
152157

153158
properties_t properties{db.connection_params(), "public"};

0 commit comments

Comments
 (0)