Skip to content

Commit 7d84fb7

Browse files
lonvialonvia
authored
Merge pull request #2035 from joto/refactor-copy-target
Refactor db_target_descr_t: Always need schema on construction and make it a real class
2 parents b22652c + 3c9dfda commit 7d84fb7

File tree

9 files changed

+88
-77
lines changed

9 files changed

+88
-77
lines changed

‎src/db-copy.cpp‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -188,24 +188,24 @@ void db_copy_thread_t::thread_t::write_to_db(db_cmd_copy_t *buffer)
188188
start_copy(buffer->target);
189189
}
190190

191-
m_conn->copy_send(buffer->buffer, buffer->target->name);
191+
m_conn->copy_send(buffer->buffer, buffer->target->name());
192192
}
193193

194194
void db_copy_thread_t::thread_t::start_copy(
195195
std::shared_ptr<db_target_descr_t> const &target)
196196
{
197197
assert(!m_inflight);
198198

199-
auto const qname = qualified_name(target->schema, target->name);
199+
auto const qname = qualified_name(target->schema(), target->name());
200200
fmt::memory_buffer sql;
201-
sql.reserve(qname.size() + target->rows.size() + 20);
202-
if (target->rows.empty()) {
201+
sql.reserve(qname.size() + target->rows().size() + 20);
202+
if (target->rows().empty()) {
203203
fmt::format_to(std::back_inserter(sql),
204204
FMT_STRING("COPY {} FROM STDIN"), qname);
205205
} else {
206206
fmt::format_to(std::back_inserter(sql),
207207
FMT_STRING("COPY {} ({}) FROM STDIN"), qname,
208-
target->rows);
208+
target->rows());
209209
}
210210

211211
sql.push_back('\0');
@@ -217,7 +217,7 @@ void db_copy_thread_t::thread_t::start_copy(
217217
void db_copy_thread_t::thread_t::finish_copy()
218218
{
219219
if (m_inflight) {
220-
m_conn->copy_end(m_inflight->name);
220+
m_conn->copy_end(m_inflight->name());
221221
m_inflight.reset();
222222
}
223223
}

‎src/db-copy.hpp‎

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
* For a full list of authors see the git log.
1111
*/
1212

13+
#include <cassert>
1314
#include <condition_variable>
1415
#include <deque>
1516
#include <future>
@@ -26,31 +27,45 @@
2627
/**
2728
* Table information necessary for building SQL queries.
2829
*/
29-
struct db_target_descr_t
30+
class db_target_descr_t
3031
{
31-
/// Schema of the target table.
32-
std::string schema{"public"};
33-
/// Name of the target table for the copy operation.
34-
std::string name;
35-
/// Name of id column used when deleting objects.
36-
std::string id;
37-
/// Comma-separated list of rows for copy operation (when empty: all rows)
38-
std::string rows;
32+
public:
33+
db_target_descr_t(std::string schema, std::string name, std::string id,
34+
std::string rows = {})
35+
: m_schema(std::move(schema)), m_name(std::move(name)), m_id(std::move(id)),
36+
m_rows(std::move(rows))
37+
{
38+
assert(!m_schema.empty());
39+
assert(!m_name.empty());
40+
assert(!m_id.empty());
41+
}
42+
43+
std::string const &schema() const noexcept { return m_schema; }
44+
std::string const &name() const noexcept { return m_name; }
45+
std::string const &id() const noexcept { return m_id; }
46+
std::string const &rows() const noexcept { return m_rows; }
47+
48+
void set_rows(std::string rows) { m_rows = std::move(rows); }
3949

4050
/**
4151
* Check if the buffer would use exactly the same copy operation.
4252
*/
4353
bool same_copy_target(db_target_descr_t const &other) const noexcept
4454
{
45-
return (this == &other) || (schema == other.schema &&
46-
name == other.name && rows == other.rows);
55+
return (this == &other) ||
56+
(m_schema == other.m_schema && m_name == other.m_name &&
57+
m_id == other.m_id && m_rows == other.m_rows);
4758
}
4859

49-
db_target_descr_t() = default;
50-
51-
db_target_descr_t(std::string n, std::string i, std::string r = {})
52-
: name(std::move(n)), id(std::move(i)), rows(std::move(r))
53-
{}
60+
private:
61+
/// Schema of the target table.
62+
std::string m_schema;
63+
/// Name of the target table for the copy operation.
64+
std::string m_name;
65+
/// Name of id column used when deleting objects.
66+
std::string m_id;
67+
/// Comma-separated list of rows for copy operation (when empty: all rows)
68+
std::string m_rows;
5469
};
5570

5671
/**
@@ -198,8 +213,9 @@ class db_cmd_copy_delete_t : public db_cmd_copy_t
198213
void delete_data(pg_conn_t *conn) override
199214
{
200215
if (m_deleter.has_data()) {
201-
m_deleter.delete_rows(qualified_name(target->schema, target->name),
202-
target->id, conn);
216+
m_deleter.delete_rows(
217+
qualified_name(target->schema(), target->name()), target->id(),
218+
conn);
203219
}
204220
}
205221

‎src/flex-table.hpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,11 +250,10 @@ class table_connection_t
250250
std::shared_ptr<db_copy_thread_t> const &copy_thread)
251251
: m_proj(reprojection::create_projection(table->srid())), m_table(table),
252252
m_target(std::make_shared<db_target_descr_t>(
253-
table->name(), table->id_column_names(),
253+
table->schema(), table->name(), table->id_column_names(),
254254
table->build_sql_column_list())),
255255
m_copy_mgr(copy_thread), m_db_connection(nullptr)
256256
{
257-
m_target->schema = table->schema();
258257
}
259258

260259
void connect(std::string const &conninfo);

‎src/gazetteer-style.hpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,10 @@ class gazetteer_copy_mgr_t : public db_copy_mgr_t<db_deleter_place_t>
8686
explicit gazetteer_copy_mgr_t(
8787
std::shared_ptr<db_copy_thread_t> const &processor)
8888
: db_copy_mgr_t<db_deleter_place_t>(processor),
89-
m_table(std::make_shared<db_target_descr_t>("place", "place_id"))
90-
{}
89+
m_table(
90+
std::make_shared<db_target_descr_t>("public", "place", "place_id"))
91+
{
92+
}
9193

9294
void prepare() { new_line(m_table); }
9395

‎src/middle-pgsql.cpp‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,9 @@ middle_pgsql_t::table_desc::table_desc(options_t const &options,
132132
table_sql const &ts)
133133
: m_create_table(build_sql(options, ts.create_table)),
134134
m_prepare_queries(build_sql(options, ts.prepare_queries)),
135-
m_copy_target(std::make_shared<db_target_descr_t>())
135+
m_copy_target(std::make_shared<db_target_descr_t>(
136+
options.middle_dbschema, build_sql(options, ts.name), "id"))
136137
{
137-
m_copy_target->name = build_sql(options, ts.name);
138-
m_copy_target->schema = options.middle_dbschema;
139-
m_copy_target->id = "id";
140-
141138
if (options.with_forward_dependencies) {
142139
m_create_fw_dep_indexes = build_sql(options, ts.create_fw_dep_indexes);
143140
}
@@ -1324,9 +1321,8 @@ void middle_pgsql_t::write_users_table()
13241321
{
13251322
log_info("Writing {} entries to table '{}'...", m_users.size(),
13261323
m_users_table.name());
1327-
auto const users_table =
1328-
std::make_shared<db_target_descr_t>(m_users_table.name(), "id");
1329-
users_table->schema = m_users_table.schema();
1324+
auto const users_table = std::make_shared<db_target_descr_t>(
1325+
m_users_table.schema(), m_users_table.name(), "id");
13301326

13311327
for (auto const &[id, name] : m_users) {
13321328
m_db_copy.new_line(users_table);

‎src/middle-pgsql.hpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,13 @@ struct middle_pgsql_t : public middle_t
133133

134134
std::string const &schema() const noexcept
135135
{
136-
return m_copy_target->schema;
136+
return m_copy_target->schema();
137137
}
138138

139-
std::string const &name() const noexcept { return m_copy_target->name; }
139+
std::string const &name() const noexcept
140+
{
141+
return m_copy_target->name();
142+
}
140143

141144
std::shared_ptr<db_target_descr_t> const &copy_target() const noexcept
142145
{

‎src/table.cpp‎

Lines changed: 28 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,11 @@ table_t::table_t(std::string const &name, std::string type, columns_t columns,
3030
hstore_column hstore_mode,
3131
std::shared_ptr<db_copy_thread_t> const &copy_thread,
3232
std::string const &schema)
33-
: m_target(std::make_shared<db_target_descr_t>(name.c_str(), "osm_id")),
33+
: m_target(std::make_shared<db_target_descr_t>(schema, name, "osm_id")),
3434
m_type(std::move(type)), m_srid(fmt::to_string(srid)), m_append(append),
3535
m_hstore_mode(hstore_mode), m_columns(std::move(columns)),
3636
m_hstore_columns(std::move(hstore_columns)), m_copy(copy_thread)
3737
{
38-
assert(!schema.empty());
39-
m_target->schema = schema;
40-
4138
// if we dont have any columns
4239
if (m_columns.empty() && m_hstore_mode != hstore_column::all) {
4340
throw fmt_error("No columns provided for table {}.", name);
@@ -78,17 +75,17 @@ void table_t::start(std::string const &conninfo, std::string const &table_space)
7875
{
7976
if (m_sql_conn) {
8077
throw fmt_error("{} cannot start, its already started.",
81-
m_target->name);
78+
m_target->name());
8279
}
8380

8481
m_conninfo = conninfo;
8582
m_table_space = tablespace_clause(table_space);
8683

8784
connect();
88-
log_info("Setting up table '{}'", m_target->name);
89-
auto const qual_name = qualified_name(m_target->schema, m_target->name);
90-
auto const qual_tmp_name = qualified_name(
91-
m_target->schema, m_target->name + "_tmp");
85+
log_info("Setting up table '{}'", m_target->name());
86+
auto const qual_name = qualified_name(m_target->schema(), m_target->name());
87+
auto const qual_tmp_name =
88+
qualified_name(m_target->schema(), m_target->name() + "_tmp");
9289

9390
// we are making a new table
9491
if (!m_append) {
@@ -135,8 +132,8 @@ void table_t::start(std::string const &conninfo, std::string const &table_space)
135132
m_sql_conn->exec(sql);
136133

137134
if (m_srid != "4326") {
138-
create_geom_check_trigger(m_sql_conn.get(), m_target->schema,
139-
m_target->name, "ST_IsValid(NEW.way)");
135+
create_geom_check_trigger(m_sql_conn.get(), m_target->schema(),
136+
m_target->name(), "ST_IsValid(NEW.way)");
140137
}
141138
}
142139

@@ -146,7 +143,7 @@ void table_t::start(std::string const &conninfo, std::string const &table_space)
146143
void table_t::prepare()
147144
{
148145
//let postgres cache this query as it will presumably happen a lot
149-
auto const qual_name = qualified_name(m_target->schema, m_target->name);
146+
auto const qual_name = qualified_name(m_target->schema(), m_target->name());
150147
m_sql_conn->exec("PREPARE get_wkb(int8) AS"
151148
" SELECT way FROM {} WHERE osm_id = $1",
152149
qual_name);
@@ -176,7 +173,7 @@ void table_t::generate_copy_column_list()
176173
// add geom column
177174
joiner.add("way");
178175

179-
m_target->rows = joiner();
176+
m_target->set_rows(joiner());
180177
}
181178

182179
void table_t::stop(bool updateable, bool enable_hstore_index,
@@ -185,17 +182,17 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
185182
// make sure that all data is written to the DB before continuing
186183
m_copy.sync();
187184

188-
auto const qual_name = qualified_name(m_target->schema, m_target->name);
189-
auto const qual_tmp_name = qualified_name(
190-
m_target->schema, m_target->name + "_tmp");
185+
auto const qual_name = qualified_name(m_target->schema(), m_target->name());
186+
auto const qual_tmp_name =
187+
qualified_name(m_target->schema(), m_target->name() + "_tmp");
191188

192189
if (!m_append) {
193190
if (m_srid != "4326") {
194-
drop_geom_check_trigger(m_sql_conn.get(), m_target->schema,
195-
m_target->name);
191+
drop_geom_check_trigger(m_sql_conn.get(), m_target->schema(),
192+
m_target->name());
196193
}
197194

198-
log_info("Clustering table '{}' by geometry...", m_target->name);
195+
log_info("Clustering table '{}' by geometry...", m_target->name());
199196

200197
std::string sql = fmt::format("CREATE TABLE {} {} AS SELECT * FROM {}",
201198
qual_tmp_name, m_table_space, qual_name);
@@ -205,7 +202,7 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
205202
sql += " ORDER BY ";
206203
if (postgis_version.major == 2 && postgis_version.minor < 4) {
207204
log_debug("Using GeoHash for clustering table '{}'",
208-
m_target->name);
205+
m_target->name());
209206
if (m_srid == "4326") {
210207
sql += "ST_GeoHash(way,10)";
211208
} else {
@@ -214,7 +211,7 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
214211
sql += " COLLATE \"C\"";
215212
} else {
216213
log_debug("Using native order for clustering table '{}'",
217-
m_target->name);
214+
m_target->name());
218215
// Since Postgis 2.4 the order function for geometries gives
219216
// useful results.
220217
sql += "way";
@@ -224,9 +221,9 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
224221

225222
m_sql_conn->exec("DROP TABLE {}", qual_name);
226223
m_sql_conn->exec(R"(ALTER TABLE {} RENAME TO "{}")", qual_tmp_name,
227-
m_target->name);
224+
m_target->name());
228225

229-
log_info("Creating geometry index on table '{}'...", m_target->name);
226+
log_info("Creating geometry index on table '{}'...", m_target->name());
230227

231228
// Use fillfactor 100 for un-updatable imports
232229
m_sql_conn->exec("CREATE INDEX ON {} USING GIST (way) {} {}", qual_name,
@@ -235,20 +232,21 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
235232

236233
/* slim mode needs this to be able to apply diffs */
237234
if (updateable) {
238-
log_info("Creating osm_id index on table '{}'...", m_target->name);
235+
log_info("Creating osm_id index on table '{}'...",
236+
m_target->name());
239237
m_sql_conn->exec("CREATE INDEX ON {} USING BTREE (osm_id) {}",
240238
qual_name, tablespace_clause(table_space_index));
241239
if (m_srid != "4326") {
242-
create_geom_check_trigger(m_sql_conn.get(), m_target->schema,
243-
m_target->name,
240+
create_geom_check_trigger(m_sql_conn.get(), m_target->schema(),
241+
m_target->name(),
244242
"ST_IsValid(NEW.way)");
245243
}
246244
}
247245

248246
/* Create hstore index if selected */
249247
if (enable_hstore_index) {
250248
log_info("Creating hstore indexes on table '{}'...",
251-
m_target->name);
249+
m_target->name());
252250
if (m_hstore_mode != hstore_column::none) {
253251
m_sql_conn->exec("CREATE INDEX ON {} USING GIN (tags) {}",
254252
qual_name,
@@ -260,8 +258,8 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
260258
tablespace_clause(table_space_index));
261259
}
262260
}
263-
log_info("Analyzing table '{}'...", m_target->name);
264-
analyze_table(*m_sql_conn, m_target->schema, m_target->name);
261+
log_info("Analyzing table '{}'...", m_target->name());
262+
analyze_table(*m_sql_conn, m_target->schema(), m_target->name());
265263
}
266264
teardown();
267265
}
@@ -372,7 +370,7 @@ void table_t::write_hstore_columns(taglist_t const &tags)
372370
void table_t::task_wait()
373371
{
374372
auto const run_time = m_task_result.wait();
375-
log_info("All postprocessing on table '{}' done in {}.", m_target->name,
373+
log_info("All postprocessing on table '{}' done in {}.", m_target->name(),
376374
util::human_readable_duration(run_time));
377375
}
378376

‎tests/test-db-copy-mgr.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,8 @@ static std::shared_ptr<db_target_descr_t> setup_table(std::string const &cols)
2727
conn.exec("CREATE TABLE test_copy_mgr (id int8{}{})",
2828
cols.empty() ? "" : ",", cols);
2929

30-
auto table = std::make_shared<db_target_descr_t>();
31-
table->name = "test_copy_mgr";
32-
table->id = "id";
30+
auto table =
31+
std::make_shared<db_target_descr_t>("public", "test_copy_mgr", "id");
3332

3433
return table;
3534
}

‎tests/test-db-copy-thread.cpp‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,8 @@ TEST_CASE("db_copy_thread_t with db_deleter_by_id_t")
2727
conn.exec("DROP TABLE IF EXISTS test_copy_thread");
2828
conn.exec("CREATE TABLE test_copy_thread (id int8)");
2929

30-
auto const table = std::make_shared<db_target_descr_t>();
31-
table->name = "test_copy_thread";
32-
table->id = "id";
30+
auto const table =
31+
std::make_shared<db_target_descr_t>("public", "test_copy_thread", "id");
3332

3433
db_copy_thread_t t(db.conninfo());
3534
using cmd_copy_t = db_cmd_copy_delete_t<db_deleter_by_id_t>;
@@ -157,9 +156,8 @@ TEST_CASE("db_copy_thread_t with db_deleter_place_t")
157156
"osm_id bigint,"
158157
"class text)");
159158

160-
auto table = std::make_shared<db_target_descr_t>();
161-
table->name = "test_copy_thread";
162-
table->id = "place_id";
159+
auto table = std::make_shared<db_target_descr_t>(
160+
"public", "test_copy_thread", "place_id");
163161

164162
db_copy_thread_t t(db.conninfo());
165163
using cmd_copy_t = db_cmd_copy_delete_t<db_deleter_place_t>;

0 commit comments

Comments
 (0)