Skip to content

Commit f720d32

Browse files
lonvialonvia
authored
Merge pull request #2500 from joto/copy-where
Replace geometry check trigger by WHERE condition on COPY
2 parents aea1218 + c3d9539 commit f720d32

10 files changed

Lines changed: 111 additions & 112 deletions

‎src/db-copy.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,11 @@ void db_copy_thread_t::thread_t::start_copy(
220220
target->rows());
221221
}
222222

223+
if (!target->conditions().empty()) {
224+
fmt::format_to(std::back_inserter(sql), FMT_STRING(" WHERE {}"),
225+
target->conditions());
226+
}
227+
223228
sql.push_back('\0');
224229
m_db_connection.copy_start(to_string(sql));
225230

‎src/db-copy.hpp‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,9 @@ class db_target_descr_t
3434
{
3535
public:
3636
db_target_descr_t(std::string schema, std::string name, std::string id,
37-
std::string rows = {})
37+
std::string rows = {}, std::string conditions = {})
3838
: m_schema(std::move(schema)), m_name(std::move(name)), m_id(std::move(id)),
39-
m_rows(std::move(rows))
39+
m_rows(std::move(rows)), m_conditions(std::move(conditions))
4040
{
4141
assert(!m_schema.empty());
4242
assert(!m_name.empty());
@@ -46,9 +46,15 @@ class db_target_descr_t
4646
std::string const &name() const noexcept { return m_name; }
4747
std::string const &id() const noexcept { return m_id; }
4848
std::string const &rows() const noexcept { return m_rows; }
49+
std::string const &conditions() const noexcept { return m_conditions; }
4950

5051
void set_rows(std::string rows) { m_rows = std::move(rows); }
5152

53+
void set_conditions(std::string conditions)
54+
{
55+
m_conditions = std::move(conditions);
56+
}
57+
5258
/**
5359
* Check if the buffer would use exactly the same copy operation.
5460
*/
@@ -68,6 +74,8 @@ class db_target_descr_t
6874
std::string m_id;
6975
/// Comma-separated list of rows for copy operation (when empty: all rows)
7076
std::string m_rows;
77+
/// Conditions for the COPY command.
78+
std::string m_conditions;
7179
};
7280

7381
/**

‎src/flex-table.cpp‎

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,27 @@ std::string flex_table_t::build_sql_column_list() const
222222
return joiner();
223223
}
224224

225+
std::string flex_table_t::build_sql_copy_condition() const
226+
{
227+
assert(!m_columns.empty());
228+
229+
std::string checks;
230+
231+
for (auto const &column : m_columns) {
232+
if (column.is_geometry_column() && column.needs_isvalid()) {
233+
checks.append(fmt::format(
234+
R"(("{0}" IS NULL OR ST_IsValid("{0}")) AND )", column.name()));
235+
}
236+
}
237+
238+
if (!checks.empty()) {
239+
// remove last " AND "
240+
checks.resize(checks.size() - 5);
241+
}
242+
243+
return checks;
244+
}
245+
225246
std::string flex_table_t::build_sql_create_id_index() const
226247
{
227248
if (m_primary_key_index) {
@@ -269,30 +290,6 @@ bool flex_table_t::with_id_cache() const noexcept { return m_with_id_cache; }
269290

270291
namespace {
271292

272-
void enable_check_trigger(pg_conn_t const &db_connection,
273-
flex_table_t const &table)
274-
{
275-
std::string checks;
276-
277-
for (auto const &column : table.columns()) {
278-
if (column.is_geometry_column() && column.needs_isvalid()) {
279-
checks.append(fmt::format(
280-
R"((NEW."{0}" IS NULL OR ST_IsValid(NEW."{0}")) AND )",
281-
column.name()));
282-
}
283-
}
284-
285-
if (checks.empty()) {
286-
return;
287-
}
288-
289-
// remove last " AND "
290-
checks.resize(checks.size() - 5);
291-
292-
create_geom_check_trigger(db_connection, table.schema(), table.name(),
293-
checks);
294-
}
295-
296293
} // anonymous namespace
297294

298295
void table_connection_t::start(pg_conn_t const &db_connection,
@@ -311,8 +308,6 @@ void table_connection_t::start(pg_conn_t const &db_connection,
311308
table().cluster_by_geom() ? flex_table_t::table_type::interim
312309
: flex_table_t::table_type::permanent,
313310
table().full_name()));
314-
315-
enable_check_trigger(db_connection, table());
316311
}
317312

318313
table().prepare(db_connection);
@@ -328,11 +323,6 @@ void table_connection_t::stop(pg_conn_t const &db_connection, bool updateable,
328323
}
329324

330325
if (table().cluster_by_geom()) {
331-
if (table().geom_column().needs_isvalid()) {
332-
drop_geom_check_trigger(db_connection, table().schema(),
333-
table().name());
334-
}
335-
336326
log_info("Clustering table '{}' by geometry...", table().name());
337327

338328
db_connection.exec(table().build_sql_create_table(
@@ -354,10 +344,6 @@ void table_connection_t::stop(pg_conn_t const &db_connection, bool updateable,
354344
db_connection.exec(R"(ALTER TABLE {} RENAME TO "{}")",
355345
table().full_tmp_name(), table().name());
356346
m_id_index_created = false;
357-
358-
if (updateable) {
359-
enable_check_trigger(db_connection, table());
360-
}
361347
}
362348

363349
if (table().indexes().empty()) {

‎src/flex-table.hpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,8 @@ class flex_table_t
157157

158158
std::string build_sql_column_list() const;
159159

160+
std::string build_sql_copy_condition() const;
161+
160162
std::string build_sql_create_id_index() const;
161163

162164
/// Does this table take objects of the specified type?
@@ -288,7 +290,7 @@ class table_connection_t
288290
: m_proj(reprojection_t::create_projection(table->srid())), m_table(table),
289291
m_target(std::make_shared<db_target_descr_t>(
290292
table->schema(), table->name(), table->id_column_names(),
291-
table->build_sql_column_list())),
293+
table->build_sql_column_list(), table->build_sql_copy_condition())),
292294
m_copy_mgr(copy_thread)
293295
{
294296
}

‎src/output-flex.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1409,6 +1409,7 @@ void output_flex_t::init_lua(std::string const &filename,
14091409
setup_lua_environment(lua_state(), filename, get_options()->append);
14101410

14111411
luaX_add_table_int(lua_state(), "stage", 1);
1412+
luaX_add_table_str(lua_state(), "proj_version", get_proj_version());
14121413

14131414
lua_pushliteral(lua_state(), "properties");
14141415
lua_createtable(lua_state(), 0, (int)properties.size());

‎src/pgsql-helper.cpp‎

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -29,47 +29,6 @@ idlist_t get_ids_from_result(pg_result_t const &result)
2929
return ids;
3030
}
3131

32-
void create_geom_check_trigger(pg_conn_t const &db_connection,
33-
std::string const &schema,
34-
std::string const &table,
35-
std::string const &condition)
36-
{
37-
std::string const func_name =
38-
qualified_name(schema, table + "_osm2pgsql_valid");
39-
40-
db_connection.exec("CREATE OR REPLACE FUNCTION {}()\n"
41-
"RETURNS TRIGGER AS $$\n"
42-
"BEGIN\n"
43-
" IF {} THEN \n"
44-
" RETURN NEW;\n"
45-
" END IF;\n"
46-
" RETURN NULL;\n"
47-
"END;"
48-
"$$ LANGUAGE plpgsql",
49-
func_name, condition);
50-
51-
db_connection.exec("CREATE TRIGGER \"{}\""
52-
" BEFORE INSERT OR UPDATE"
53-
" ON {}"
54-
" FOR EACH ROW EXECUTE PROCEDURE"
55-
" {}()",
56-
table + "_osm2pgsql_valid",
57-
qualified_name(schema, table), func_name);
58-
}
59-
60-
void drop_geom_check_trigger(pg_conn_t const &db_connection,
61-
std::string const &schema,
62-
std::string const &table)
63-
{
64-
std::string const func_name =
65-
qualified_name(schema, table + "_osm2pgsql_valid");
66-
67-
db_connection.exec(R"(DROP TRIGGER "{}" ON {})", table + "_osm2pgsql_valid",
68-
qualified_name(schema, table));
69-
70-
db_connection.exec("DROP FUNCTION IF EXISTS {} ()", func_name);
71-
}
72-
7332
void analyze_table(pg_conn_t const &db_connection, std::string const &schema,
7433
std::string const &name)
7534
{

‎src/pgsql-helper.hpp‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,6 @@ class pg_result_t;
2727
*/
2828
idlist_t get_ids_from_result(pg_result_t const &result);
2929

30-
void create_geom_check_trigger(pg_conn_t const &db_connection,
31-
std::string const &schema,
32-
std::string const &table,
33-
std::string const &condition);
34-
35-
void drop_geom_check_trigger(pg_conn_t const &db_connection,
36-
std::string const &schema,
37-
std::string const &table);
38-
3930
void analyze_table(pg_conn_t const &db_connection, std::string const &schema,
4031
std::string const &name);
4132

‎src/table.cpp‎

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -129,11 +129,6 @@ void table_t::start(connection_params_t const &connection_params,
129129

130130
//create the table
131131
m_db_connection->exec(sql);
132-
133-
if (m_srid != "4326") {
134-
create_geom_check_trigger(*m_db_connection, m_target->schema(),
135-
m_target->name(), "ST_IsValid(NEW.way)");
136-
}
137132
}
138133

139134
prepare();
@@ -172,6 +167,10 @@ void table_t::generate_copy_column_list()
172167
joiner.add("way");
173168

174169
m_target->set_rows(joiner());
170+
171+
if (m_srid != "4326") {
172+
m_target->set_conditions("ST_IsValid(way)");
173+
}
175174
}
176175

177176
void table_t::stop(bool updateable, bool enable_hstore_index,
@@ -185,11 +184,6 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
185184
qualified_name(m_target->schema(), m_target->name() + "_tmp");
186185

187186
if (!m_append) {
188-
if (m_srid != "4326") {
189-
drop_geom_check_trigger(*m_db_connection, m_target->schema(),
190-
m_target->name());
191-
}
192-
193187
log_info("Clustering table '{}' by geometry...", m_target->name());
194188

195189
std::string const sql =
@@ -217,11 +211,6 @@ void table_t::stop(bool updateable, bool enable_hstore_index,
217211
m_db_connection->exec("CREATE INDEX ON {} USING BTREE (osm_id) {}",
218212
qual_name,
219213
tablespace_clause(table_space_index));
220-
if (m_srid != "4326") {
221-
create_geom_check_trigger(*m_db_connection, m_target->schema(),
222-
m_target->name(),
223-
"ST_IsValid(NEW.way)");
224-
}
225214
}
226215

227216
/* Create hstore index if selected */

‎tests/bdd/flex/invalid-after-projection.feature‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
Feature: A valid geometry that is projected can become invalid
2+
3+
Scenario: Valid geometry must end up in output table
4+
Given the OSM data
5+
"""
6+
n10 t2020-01-02T03:04:05Z x0 y50
7+
n11 t2020-01-02T03:04:05Z x50 y50
8+
n12 t2020-01-02T03:04:05Z x100 y50
9+
n13 t2020-01-02T03:04:05Z x100 y50.1
10+
n14 t2020-01-02T03:04:05Z x0 y50.1
11+
w20 t2020-01-02T03:04:06Z Tlanduse=forest Nn10,n11,n12,n13,n14,n10
12+
"""
13+
And the lua style
14+
"""
15+
local polygons = osm2pgsql.define_table({
16+
name = 'osm2pgsql_test_polygon',
17+
ids = { type = 'way', id_column = 'osm_id' },
18+
columns = {
19+
{ column = 'geom', type = 'polygon', projection = 4326 },
20+
}
21+
})
22+
function osm2pgsql.process_way(object)
23+
polygons:insert({
24+
geom = object:as_polygon()
25+
})
26+
end
27+
"""
28+
When running osm2pgsql flex
29+
Then table osm2pgsql_test_polygon has 1 rows
30+
31+
Scenario: Invalid geometry after projection must not end up in output table
32+
Given the OSM data
33+
"""
34+
n10 t2020-01-02T03:04:05Z x0 y50
35+
n11 t2020-01-02T03:04:05Z x50 y50
36+
n12 t2020-01-02T03:04:05Z x100 y50
37+
n13 t2020-01-02T03:04:05Z x100 y50.1
38+
n14 t2020-01-02T03:04:05Z x0 y50.1
39+
w20 t2020-01-02T03:04:06Z Tlanduse=forest Nn10,n11,n12,n13,n14,n10
40+
"""
41+
And the lua style
42+
"""
43+
local srid = 4326
44+
45+
if osm2pgsql.proj_version ~= '[disabled]' then
46+
srid = 3031
47+
end
48+
49+
local polygons = osm2pgsql.define_table({
50+
name = 'osm2pgsql_test_polygon',
51+
ids = { type = 'way', id_column = 'osm_id' },
52+
columns = {
53+
{ column = 'geom', type = 'polygon', projection = srid },
54+
}
55+
})
56+
57+
if osm2pgsql.proj_version ~= '[disabled]' then
58+
function osm2pgsql.process_way(object)
59+
polygons:insert({
60+
geom = object:as_polygon()
61+
})
62+
end
63+
end
64+
"""
65+
When running osm2pgsql flex
66+
Then table osm2pgsql_test_polygon has 0 rows
67+

‎tests/test-output-flex-schema.cpp‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,4 @@ TEST_CASE("config with schema should work")
3737
conn.get_count("pg_catalog.pg_tables", "schemaname = 'myschema'"));
3838

3939
REQUIRE(7103 == conn.get_count("myschema.osm2pgsql_test_line"));
40-
41-
REQUIRE(1 ==
42-
conn.get_count("pg_catalog.pg_proc",
43-
"proname = 'osm2pgsql_test_line_osm2pgsql_valid'"));
44-
45-
REQUIRE(1 == conn.get_count("pg_catalog.pg_trigger"));
46-
REQUIRE(1 ==
47-
conn.get_count("pg_catalog.pg_trigger",
48-
"tgname = 'osm2pgsql_test_line_osm2pgsql_valid'"));
4940
}

0 commit comments

Comments
 (0)