From 3b95ca43b3042c0d1f8e135df44d7e05050ff70d Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Fri, 3 Jul 2026 14:42:45 +0200 Subject: [PATCH] Fix a bug in diff expire handling If you have multiple expire outputs on a single geometry column and at least one of them is configured as a diff_expire, osm2pgsql can segfault. This commit fixes that problem. The problem was that to calculate the diff geometries, the original geometries were destroyed which lead to the segfault when those geometries were accessed again later. Now we make sure to access the original geometries only until we generate the diff geometries and after that only access the diff geometries. As a side effect processing will even be faster if you have several expire configs that use diff processing. --- src/flex-table-column.cpp | 107 +++++++++++++++-------------- tests/bdd/flex/expire-diff.feature | 86 +++++++++++++++++++++++ 2 files changed, 142 insertions(+), 51 deletions(-) diff --git a/src/flex-table-column.cpp b/src/flex-table-column.cpp index b1be115e4..98a9cd4e1 100644 --- a/src/flex-table-column.cpp +++ b/src/flex-table-column.cpp @@ -214,24 +214,6 @@ void flex_table_column_t::add_expire(expire_config_t const &config) } namespace { - -/** - * This expires all geometries in "geoms" by themselves. Used when we don't - * need diff expire. - */ -void separate_expire(std::vector const &geoms, - expire_config_t const &expire_config, - expire_tiles_t &expire_tiles, - std::vector *expire_outputs) -{ - assert(expire_outputs); - - for (auto const &geom : geoms) { - expire_tiles.from_geometry(geom, expire_config); - } - expire_tiles.commit_tiles(&expire_outputs->at(expire_config.expire_output)); -} - /** * When doing diff expire, we need to calculate the symmetric difference * between old and new geometries. The difference is done by type, so points @@ -282,28 +264,14 @@ void classify_geometries(T input_geoms, geom::multipoint_t *points, } // NOLINTEND(cppcoreguidelines-rvalue-reference-param-not-moved) -template -void diff_and_expire(geom::multigeometry_t const &old_geoms, - geom::multigeometry_t const &new_geoms, - expire_config_t const &expire_config, - expire_tiles_t &expire_tiles) -{ - std::vector diffs; - boost::geometry::sym_difference(old_geoms, new_geoms, diffs); - for (auto const &geom : diffs) { - expire_tiles.from_geometry(geom, expire_config); - } -} - -void diff_expire(std::vector *geoms_old, - std::vector *geoms_new, - expire_config_t const &expire_config, - expire_tiles_t &expire_tiles, - std::vector *expire_outputs) +void find_difference(std::vector *geoms_old, + std::vector *geoms_new, + std::vector *diff_points, + std::vector *diff_linestrings, + std::vector *diff_polygons) { assert(geoms_old); assert(geoms_new); - assert(expire_outputs); geom::multipoint_t old_points; geom::multilinestring_t old_linestrings; @@ -319,12 +287,10 @@ void diff_expire(std::vector *geoms_old, classify_geometries(geoms_new, &new_points, &new_linestrings, &new_polygons); - diff_and_expire(old_points, new_points, expire_config, expire_tiles); - diff_and_expire(old_linestrings, new_linestrings, expire_config, - expire_tiles); - diff_and_expire(old_polygons, new_polygons, expire_config, expire_tiles); - - expire_tiles.commit_tiles(&expire_outputs->at(expire_config.expire_output)); + boost::geometry::sym_difference(old_points, new_points, *diff_points); + boost::geometry::sym_difference(old_linestrings, new_linestrings, + *diff_linestrings); + boost::geometry::sym_difference(old_polygons, new_polygons, *diff_polygons); } } // anonymous namespace @@ -340,19 +306,58 @@ void flex_table_column_t::do_expire( assert(expire); assert(expire_outputs); + // Sometimes it doesn't depend on the expire config whether we want to + // do diff expire. + bool const always_separate = + !enable_diff_expire || geoms_old->empty() || geoms_new->empty(); + + bool need_diff_expire = false; + for (auto const &expire_config : m_expires) { assert(expire_config.expire_output < expire->size()); auto &expire_tiles = expire->at(expire_config.expire_output); - if (!expire_config.diff_expire || !enable_diff_expire || - geoms_old->empty() || geoms_new->empty()) { - separate_expire(*geoms_old, expire_config, expire_tiles, - expire_outputs); - separate_expire(*geoms_new, expire_config, expire_tiles, - expire_outputs); + if (!expire_config.diff_expire || always_separate) { + for (auto const &geom : *geoms_old) { + expire_tiles.from_geometry(geom, expire_config); + } + for (auto const &geom : *geoms_new) { + expire_tiles.from_geometry(geom, expire_config); + } + expire_tiles.commit_tiles( + &expire_outputs->at(expire_config.expire_output)); } else { - diff_expire(geoms_old, geoms_new, expire_config, expire_tiles, - expire_outputs); + need_diff_expire = true; + } + } + + if (always_separate || !need_diff_expire) { + return; + } + + std::vector diff_points; + std::vector diff_linestrings; + std::vector diff_polygons; + + find_difference(geoms_old, geoms_new, &diff_points, &diff_linestrings, + &diff_polygons); + + for (auto const &expire_config : m_expires) { + assert(expire_config.expire_output < expire->size()); + auto &expire_tiles = expire->at(expire_config.expire_output); + + if (expire_config.diff_expire) { + for (auto const &geom : diff_points) { + expire_tiles.from_geometry(geom, expire_config); + } + for (auto const &geom : diff_linestrings) { + expire_tiles.from_geometry(geom, expire_config); + } + for (auto const &geom : diff_polygons) { + expire_tiles.from_geometry(geom, expire_config); + } + expire_tiles.commit_tiles( + &expire_outputs->at(expire_config.expire_output)); } } } diff --git a/tests/bdd/flex/expire-diff.feature b/tests/bdd/flex/expire-diff.feature index 28f2d6254..292a90b26 100644 --- a/tests/bdd/flex/expire-diff.feature +++ b/tests/bdd/flex/expire-diff.feature @@ -359,3 +359,89 @@ Feature: Diff expire | 8 | 129 | 126 | | 8 | 130 | 126 | + Scenario: multiple expire configs, with and without diff expire + Given the OSM data + """ + n1 v1 x0 y0 + n2 v1 x2 y0 + n3 v1 x2 y1 + n4 v1 x4 y1 + w1 v1 Thighway=primary Nn1,n2,n3,n4 + """ + And the lua style + """ + local eo1 = osm2pgsql.define_expire_output({ + table = 'osm2pgsql_test_expire1', + maxzoom = 8, + }) + + local eo2 = osm2pgsql.define_expire_output({ + table = 'osm2pgsql_test_expire2', + maxzoom = 8, + }) + + local eo3 = osm2pgsql.define_expire_output({ + table = 'osm2pgsql_test_expire3', + maxzoom = 7, + }) + + local the_table = osm2pgsql.define_way_table('osm2pgsql_test', { + { column = 'geom', type = 'linestring', expire = { + { output = eo1, diff_expire = true }, + { output = eo2, diff_expire = false }, + { output = eo3, diff_expire = true } + } + }, + }) + + function osm2pgsql.process_way(object) + the_table:insert{ + geom = object:as_linestring() + } + end + """ + When running osm2pgsql flex with parameters + | --slim | -c | + Then table osm2pgsql_test has 1 rows + Then table osm2pgsql_test contains exactly + | way_id | geom!geo | + | 1 | 0 0,222638.98158654713 0,222638.98158654713 111325.14285463623,445277.96317309426 111325.14285463623 | + Then table osm2pgsql_test_expire1 has 0 rows + Then table osm2pgsql_test_expire2 has 0 rows + Then table osm2pgsql_test_expire3 has 0 rows + + Given the OSM data + """ + n2 v2 x0 y1 + """ + When running osm2pgsql flex with parameters + | --slim | -a | + Then table osm2pgsql_test contains exactly + | way_id | geom!geo | + | 1 | 0 0,0 111325.14285463623,222638.98158654713 111325.14285463623,445277.96317309426 111325.14285463623 | + Then table osm2pgsql_test_expire1 contains exactly + | zoom | x | y | + | 8 | 127 | 127 | + | 8 | 128 | 127 | + | 8 | 129 | 127 | + | 8 | 127 | 128 | + | 8 | 128 | 128 | + | 8 | 129 | 128 | + + Then table osm2pgsql_test_expire2 contains exactly + | zoom | x | y | + | 8 | 127 | 127 | + | 8 | 128 | 127 | + | 8 | 129 | 127 | + | 8 | 130 | 127 | + | 8 | 127 | 128 | + | 8 | 128 | 128 | + | 8 | 129 | 128 | + + Then table osm2pgsql_test_expire3 contains exactly + | zoom | x | y | + | 7 | 63 | 63 | + | 7 | 63 | 64 | + | 7 | 64 | 63 | + | 7 | 64 | 64 | +