From e9efef90954cba09fdba3e556e9efff75540071c Mon Sep 17 00:00:00 2001 From: lujoh Date: Thu, 12 Oct 2023 22:00:43 -0400 Subject: [PATCH 1/5] added subcommand to clean deleted relations for issue # 2444 --- nominatim/clicmd/admin.py | 13 +++++ nominatim/clicmd/args.py | 2 + nominatim/tools/admin.py | 21 +++++++ test/python/cli/test_cmd_admin.py | 6 ++ test/python/tools/test_admin.py | 91 +++++++++++++++++++++++++++++++ 5 files changed, 133 insertions(+) diff --git a/nominatim/clicmd/admin.py b/nominatim/clicmd/admin.py index 0f498ad2..debbd8d3 100644 --- a/nominatim/clicmd/admin.py +++ b/nominatim/clicmd/admin.py @@ -41,6 +41,8 @@ class AdminFuncs: help='Print performance analysis of the indexing process') objs.add_argument('--collect-os-info', action="store_true", help="Generate a report about the host system information") + objs.add_argument('--clean-deleted', action='store_true', + help='Clean up deleted relations') group = parser.add_argument_group('Arguments for cache warming') group.add_argument('--search-only', action='store_const', dest='target', const='search', @@ -54,8 +56,13 @@ class AdminFuncs: help='Analyse indexing of the given OSM object') mgroup.add_argument('--place-id', type=int, help='Analyse indexing of the given Nominatim object') + group = parser.add_argument_group('Arguments for cleaning deleted') + group.add_argument('--age', type=str, + help='Delete relations older than the given PostgreSQL time interval') + def run(self, args: NominatimArgs) -> int: + # pylint: disable=too-many-return-statements if args.warm: return self._warm(args) @@ -81,6 +88,12 @@ class AdminFuncs: collect_os_info.report_system_information(args.config) return 0 + if args.clean_deleted: + LOG.warning('Cleaning up deleted relations') + from ..tools import admin + admin.clean_deleted_relations(args.config, age=args.age) + return 0 + return 1 diff --git a/nominatim/clicmd/args.py b/nominatim/clicmd/args.py index 8b805496..5b4beb40 100644 --- a/nominatim/clicmd/args.py +++ b/nominatim/clicmd/args.py @@ -72,10 +72,12 @@ class NominatimArgs: check_database: bool migrate: bool collect_os_info: bool + clean_deleted: bool analyse_indexing: bool target: Optional[str] osm_id: Optional[str] place_id: Optional[int] + age: str # Arguments to 'import' osm_file: List[str] diff --git a/nominatim/tools/admin.py b/nominatim/tools/admin.py index da7845eb..f27d9bbc 100644 --- a/nominatim/tools/admin.py +++ b/nominatim/tools/admin.py @@ -11,6 +11,7 @@ from typing import Optional, Tuple, Any, cast import logging from psycopg2.extras import Json, register_hstore +from psycopg2 import DataError from nominatim.config import Configuration from nominatim.db.connection import connect, Cursor @@ -87,3 +88,23 @@ def analyse_indexing(config: Configuration, osm_id: Optional[str] = None, for msg in conn.notices: print(msg) + + +def clean_deleted_relations(config: Configuration, age: Optional[str] = None) -> None: + """ Clean deleted relations older than a given age + """ + if not age: + LOG.fatal('No age given to delete relations') + raise UsageError('Age parameter not found') + with connect(config.get_libpq_dsn()) as conn: + with conn.cursor() as cur: + try: + cur.execute("""SELECT place_force_delete(p.place_id) + FROM import_polygon_delete d, placex p + WHERE p.osm_type = d.osm_type AND p.osm_id = d.osm_id + AND age(p.indexed_date) > %s::interval""", + (age, )) + except DataError as exc: + raise UsageError('Invalid PostgreSQL time interval format') from exc + conn.commit() + \ No newline at end of file diff --git a/test/python/cli/test_cmd_admin.py b/test/python/cli/test_cmd_admin.py index 75ae3cd2..20e5be00 100644 --- a/test/python/cli/test_cmd_admin.py +++ b/test/python/cli/test_cmd_admin.py @@ -33,6 +33,12 @@ def test_admin_migrate(cli_call, mock_func_factory): assert mock.called == 1 +def test_admin_clean_deleted_relations(cli_call, mock_func_factory): + mock = mock_func_factory(nominatim.tools.admin, 'clean_deleted_relations') + + assert cli_call('admin', '--clean-deleted') == 0 + assert mock.called == 1 + class TestCliAdminWithDb: @pytest.fixture(autouse=True) diff --git a/test/python/tools/test_admin.py b/test/python/tools/test_admin.py index 9c010b9d..dd96c943 100644 --- a/test/python/tools/test_admin.py +++ b/test/python/tools/test_admin.py @@ -70,3 +70,94 @@ def test_analyse_indexing_with_osm_id(project_env, temp_db_cursor): VALUES(9988, 'N', 10000)""") admin.analyse_indexing(project_env, osm_id='N10000') + + +class TestAdminCleanDeleted: + + @pytest.fixture(autouse=True) + def setup_polygon_delete(self, project_env, table_factory, temp_db_cursor): + """ Set up import_polygon_delete table and simplified place_force_delete function + """ + self.project_env = project_env + self.temp_db_cursor = temp_db_cursor + table_factory('import_polygon_delete', + """osm_id BIGINT, + osm_type CHAR(1), + class TEXT NOT NULL, + type TEXT NOT NULL""", + ((100, 'N', 'boundary', 'administrative'), + (145, 'N', 'boundary', 'administrative'), + (175, 'R', 'landcover', 'grass'))) + table_factory('place_to_be_deleted', + """osm_id BIGINT, + osm_type CHAR(1), + class TEXT NOT NULL, + type TEXT NOT NULL, + deferred BOOLEAN""") + temp_db_cursor.execute("""INSERT INTO placex (place_id, osm_id, osm_type, class, type, indexed_date, indexed_status) + VALUES(1, 100, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), + (2, 145, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), + (3, 175, 'R', 'landcover', 'grass', current_date - INTERVAL '1 month', 1)""") + temp_db_cursor.execute("""CREATE OR REPLACE FUNCTION flush_deleted_places() + RETURNS INTEGER + AS $$ + BEGIN + UPDATE placex p SET indexed_status = 100 FROM place_to_be_deleted d + WHERE p.osm_type = d.osm_type + AND p.osm_id = d.osm_id + AND p.class = d.class + AND p.type = d.type + AND NOT deferred; + TRUNCATE TABLE place_to_be_deleted; + RETURN NULL; + END; + $$ + LANGUAGE plpgsql; + """) + temp_db_cursor.execute("""CREATE OR REPLACE FUNCTION place_force_delete(placeid BIGINT) + RETURNS BOOLEAN + AS $$ + DECLARE + osmid BIGINT; + osmtype character(1); + pclass text; + ptype text; + BEGIN + SELECT osm_type, osm_id, class, type FROM placex WHERE place_id = placeid INTO osmtype, osmid, pclass, ptype; + DELETE FROM import_polygon_delete WHERE osm_type = osmtype AND osm_id = osmid AND class = pclass AND type = ptype; + INSERT INTO place_to_be_deleted (osm_type, osm_id, class, type, deferred) + VALUES(osmtype, osmid, pclass, ptype, false); + PERFORM flush_deleted_places(); + RETURN TRUE; + END; + $$ + LANGUAGE plpgsql; + """) + + + def test_admin_clean_deleted_no_records(self): + admin.clean_deleted_relations(self.project_env, age='1 year') + assert self.temp_db_cursor.row_set('SELECT osm_id, osm_type, class, type, indexed_status FROM placex') == {(100, 'N', 'boundary', 'administrative', 1), + (145, 'N', 'boundary', 'administrative', 1), + (175, 'R', 'landcover', 'grass', 1)} + assert self.temp_db_cursor.table_rows('import_polygon_delete') == 3 + + + def test_admin_clean_deleted_no_age(self): + with pytest.raises(UsageError): + admin.clean_deleted_relations(self.project_env) + + + @pytest.mark.parametrize('test_age', ['T week', '1 welk', 'P1E']) + def test_admin_clean_deleted_bad_age(self, test_age): + with pytest.raises(UsageError): + admin.clean_deleted_relations(self.project_env, age = test_age) + + + @pytest.mark.parametrize('test_age', ['1 week', 'P3D', '5 hours']) + def test_admin_clean_deleted(self, test_age): + admin.clean_deleted_relations(self.project_env, age = test_age) + assert self.temp_db_cursor.row_set('SELECT osm_id, osm_type, class, type, indexed_status FROM placex') == {(100, 'N', 'boundary', 'administrative', 100), + (145, 'N', 'boundary', 'administrative', 100), + (175, 'R', 'landcover', 'grass', 100)} + assert self.temp_db_cursor.table_rows('import_polygon_delete') == 0 From 06204dfcd861bc46547bfbb670dd7ce2e6ef3876 Mon Sep 17 00:00:00 2001 From: lujoh Date: Tue, 17 Oct 2023 18:22:27 -0400 Subject: [PATCH 2/5] moved sql function flush_deleted_places() to utils --- lib-sql/functions/place_triggers.sql | 54 ----------------------- lib-sql/functions/utils.sql | 53 +++++++++++++++++++++++ test/python/tools/test_admin.py | 65 ++++++++++------------------ 3 files changed, 77 insertions(+), 95 deletions(-) diff --git a/lib-sql/functions/place_triggers.sql b/lib-sql/functions/place_triggers.sql index 3def6596..f3b6ab2b 100644 --- a/lib-sql/functions/place_triggers.sql +++ b/lib-sql/functions/place_triggers.sql @@ -363,57 +363,3 @@ BEGIN RETURN NULL; END; $$ LANGUAGE plpgsql; - -CREATE OR REPLACE FUNCTION flush_deleted_places() - RETURNS INTEGER - AS $$ -BEGIN - -- deleting large polygons can have a massive effect on the system - require manual intervention to let them through - INSERT INTO import_polygon_delete (osm_type, osm_id, class, type) - SELECT osm_type, osm_id, class, type FROM place_to_be_deleted WHERE deferred; - - -- delete from place table - ALTER TABLE place DISABLE TRIGGER place_before_delete; - DELETE FROM place USING place_to_be_deleted - WHERE place.osm_type = place_to_be_deleted.osm_type - and place.osm_id = place_to_be_deleted.osm_id - and place.class = place_to_be_deleted.class - and place.type = place_to_be_deleted.type - and not deferred; - ALTER TABLE place ENABLE TRIGGER place_before_delete; - - -- Mark for delete in the placex table - UPDATE placex SET indexed_status = 100 FROM place_to_be_deleted - WHERE placex.osm_type = 'N' and place_to_be_deleted.osm_type = 'N' - and placex.osm_id = place_to_be_deleted.osm_id - and placex.class = place_to_be_deleted.class - and placex.type = place_to_be_deleted.type - and not deferred; - UPDATE placex SET indexed_status = 100 FROM place_to_be_deleted - WHERE placex.osm_type = 'W' and place_to_be_deleted.osm_type = 'W' - and placex.osm_id = place_to_be_deleted.osm_id - and placex.class = place_to_be_deleted.class - and placex.type = place_to_be_deleted.type - and not deferred; - UPDATE placex SET indexed_status = 100 FROM place_to_be_deleted - WHERE placex.osm_type = 'R' and place_to_be_deleted.osm_type = 'R' - and placex.osm_id = place_to_be_deleted.osm_id - and placex.class = place_to_be_deleted.class - and placex.type = place_to_be_deleted.type - and not deferred; - - -- Mark for delete in interpolations - UPDATE location_property_osmline SET indexed_status = 100 FROM place_to_be_deleted - WHERE place_to_be_deleted.osm_type = 'W' - and place_to_be_deleted.class = 'place' - and place_to_be_deleted.type = 'houses' - and location_property_osmline.osm_id = place_to_be_deleted.osm_id - and not deferred; - - -- Clear todo list. - TRUNCATE TABLE place_to_be_deleted; - - RETURN NULL; -END; -$$ LANGUAGE plpgsql; - diff --git a/lib-sql/functions/utils.sql b/lib-sql/functions/utils.sql index b2771ba1..ff2f037d 100644 --- a/lib-sql/functions/utils.sql +++ b/lib-sql/functions/utils.sql @@ -487,3 +487,56 @@ BEGIN END; $$ LANGUAGE plpgsql; + +CREATE OR REPLACE FUNCTION flush_deleted_places() + RETURNS INTEGER + AS $$ +BEGIN + -- deleting large polygons can have a massive effect on the system - require manual intervention to let them through + INSERT INTO import_polygon_delete (osm_type, osm_id, class, type) + SELECT osm_type, osm_id, class, type FROM place_to_be_deleted WHERE deferred; + + -- delete from place table + ALTER TABLE place DISABLE TRIGGER place_before_delete; + DELETE FROM place USING place_to_be_deleted + WHERE place.osm_type = place_to_be_deleted.osm_type + and place.osm_id = place_to_be_deleted.osm_id + and place.class = place_to_be_deleted.class + and place.type = place_to_be_deleted.type + and not deferred; + ALTER TABLE place ENABLE TRIGGER place_before_delete; + + -- Mark for delete in the placex table + UPDATE placex SET indexed_status = 100 FROM place_to_be_deleted + WHERE placex.osm_type = 'N' and place_to_be_deleted.osm_type = 'N' + and placex.osm_id = place_to_be_deleted.osm_id + and placex.class = place_to_be_deleted.class + and placex.type = place_to_be_deleted.type + and not deferred; + UPDATE placex SET indexed_status = 100 FROM place_to_be_deleted + WHERE placex.osm_type = 'W' and place_to_be_deleted.osm_type = 'W' + and placex.osm_id = place_to_be_deleted.osm_id + and placex.class = place_to_be_deleted.class + and placex.type = place_to_be_deleted.type + and not deferred; + UPDATE placex SET indexed_status = 100 FROM place_to_be_deleted + WHERE placex.osm_type = 'R' and place_to_be_deleted.osm_type = 'R' + and placex.osm_id = place_to_be_deleted.osm_id + and placex.class = place_to_be_deleted.class + and placex.type = place_to_be_deleted.type + and not deferred; + + -- Mark for delete in interpolations + UPDATE location_property_osmline SET indexed_status = 100 FROM place_to_be_deleted + WHERE place_to_be_deleted.osm_type = 'W' + and place_to_be_deleted.class = 'place' + and place_to_be_deleted.type = 'houses' + and location_property_osmline.osm_id = place_to_be_deleted.osm_id + and not deferred; + + -- Clear todo list. + TRUNCATE TABLE place_to_be_deleted; + + RETURN NULL; +END; +$$ LANGUAGE plpgsql; diff --git a/test/python/tools/test_admin.py b/test/python/tools/test_admin.py index dd96c943..3ce3c8f4 100644 --- a/test/python/tools/test_admin.py +++ b/test/python/tools/test_admin.py @@ -12,6 +12,7 @@ import pytest from nominatim.errors import UsageError from nominatim.tools import admin from nominatim.tokenizer import factory +from nominatim.db.sql_preprocessor import SQLPreprocessor @pytest.fixture(autouse=True) def create_placex_table(project_env, tokenizer_mock, temp_db_cursor, placex_table): @@ -75,8 +76,8 @@ def test_analyse_indexing_with_osm_id(project_env, temp_db_cursor): class TestAdminCleanDeleted: @pytest.fixture(autouse=True) - def setup_polygon_delete(self, project_env, table_factory, temp_db_cursor): - """ Set up import_polygon_delete table and simplified place_force_delete function + def setup_polygon_delete(self, project_env, table_factory, place_table, osmline_table, temp_db_cursor, temp_db_conn, def_config, src_dir): + """ Set up place_force_delete function and related tables """ self.project_env = project_env self.temp_db_cursor = temp_db_cursor @@ -88,51 +89,33 @@ class TestAdminCleanDeleted: ((100, 'N', 'boundary', 'administrative'), (145, 'N', 'boundary', 'administrative'), (175, 'R', 'landcover', 'grass'))) + temp_db_cursor.execute("""INSERT INTO placex (place_id, osm_id, osm_type, class, type, indexed_date, indexed_status) + VALUES(1, 100, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), + (2, 145, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), + (3, 175, 'R', 'landcover', 'grass', current_date - INTERVAL '1 month', 1)""") + # set up tables and triggers for utils function table_factory('place_to_be_deleted', """osm_id BIGINT, osm_type CHAR(1), class TEXT NOT NULL, type TEXT NOT NULL, deferred BOOLEAN""") - temp_db_cursor.execute("""INSERT INTO placex (place_id, osm_id, osm_type, class, type, indexed_date, indexed_status) - VALUES(1, 100, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), - (2, 145, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), - (3, 175, 'R', 'landcover', 'grass', current_date - INTERVAL '1 month', 1)""") - temp_db_cursor.execute("""CREATE OR REPLACE FUNCTION flush_deleted_places() - RETURNS INTEGER - AS $$ - BEGIN - UPDATE placex p SET indexed_status = 100 FROM place_to_be_deleted d - WHERE p.osm_type = d.osm_type - AND p.osm_id = d.osm_id - AND p.class = d.class - AND p.type = d.type - AND NOT deferred; - TRUNCATE TABLE place_to_be_deleted; - RETURN NULL; - END; - $$ - LANGUAGE plpgsql; - """) - temp_db_cursor.execute("""CREATE OR REPLACE FUNCTION place_force_delete(placeid BIGINT) - RETURNS BOOLEAN - AS $$ - DECLARE - osmid BIGINT; - osmtype character(1); - pclass text; - ptype text; - BEGIN - SELECT osm_type, osm_id, class, type FROM placex WHERE place_id = placeid INTO osmtype, osmid, pclass, ptype; - DELETE FROM import_polygon_delete WHERE osm_type = osmtype AND osm_id = osmid AND class = pclass AND type = ptype; - INSERT INTO place_to_be_deleted (osm_type, osm_id, class, type, deferred) - VALUES(osmtype, osmid, pclass, ptype, false); - PERFORM flush_deleted_places(); - RETURN TRUE; - END; - $$ - LANGUAGE plpgsql; - """) + table_factory('country_name', 'partition INT') + table_factory('import_polygon_error', """osm_id BIGINT, + osm_type CHAR(1), + class TEXT NOT NULL, + type TEXT NOT NULL""") + temp_db_cursor.execute("""CREATE OR REPLACE FUNCTION place_delete() + RETURNS TRIGGER AS $$ + BEGIN RETURN NULL; END; + $$ LANGUAGE plpgsql;""") + temp_db_cursor.execute("""CREATE TRIGGER place_before_delete BEFORE DELETE ON place + FOR EACH ROW EXECUTE PROCEDURE place_delete();""") + orig_sql = def_config.lib_dir.sql + def_config.lib_dir.sql = src_dir / 'lib-sql' + sqlproc = SQLPreprocessor(temp_db_conn, def_config) + sqlproc.run_sql_file(temp_db_conn, 'functions/utils.sql') + def_config.lib_dir.sql = orig_sql def test_admin_clean_deleted_no_records(self): From 9ec26c60ff1c9b5b259ecb7ef16ad0bacbc3174b Mon Sep 17 00:00:00 2001 From: lujoh Date: Tue, 17 Oct 2023 23:03:37 -0400 Subject: [PATCH 3/5] adjusted tests for --clean-deleted-relations command --- nominatim/clicmd/admin.py | 3 +++ nominatim/tools/admin.py | 5 +---- test/python/cli/test_cmd_admin.py | 8 +++++++- test/python/tools/test_admin.py | 16 +++++++++------- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/nominatim/clicmd/admin.py b/nominatim/clicmd/admin.py index debbd8d3..9a7b92e6 100644 --- a/nominatim/clicmd/admin.py +++ b/nominatim/clicmd/admin.py @@ -29,6 +29,7 @@ class AdminFuncs: """ def add_args(self, parser: argparse.ArgumentParser) -> None: + self.parser = parser group = parser.add_argument_group('Admin tasks') objs = group.add_mutually_exclusive_group(required=True) objs.add_argument('--warm', action='store_true', @@ -89,6 +90,8 @@ class AdminFuncs: return 0 if args.clean_deleted: + if not args.age: + self.parser.error('Age is required for --clean-deleted command') LOG.warning('Cleaning up deleted relations') from ..tools import admin admin.clean_deleted_relations(args.config, age=args.age) diff --git a/nominatim/tools/admin.py b/nominatim/tools/admin.py index f27d9bbc..5bc3954b 100644 --- a/nominatim/tools/admin.py +++ b/nominatim/tools/admin.py @@ -90,12 +90,9 @@ def analyse_indexing(config: Configuration, osm_id: Optional[str] = None, print(msg) -def clean_deleted_relations(config: Configuration, age: Optional[str] = None) -> None: +def clean_deleted_relations(config: Configuration, age: str) -> None: """ Clean deleted relations older than a given age """ - if not age: - LOG.fatal('No age given to delete relations') - raise UsageError('Age parameter not found') with connect(config.get_libpq_dsn()) as conn: with conn.cursor() as cur: try: diff --git a/test/python/cli/test_cmd_admin.py b/test/python/cli/test_cmd_admin.py index 20e5be00..0fe6e327 100644 --- a/test/python/cli/test_cmd_admin.py +++ b/test/python/cli/test_cmd_admin.py @@ -36,9 +36,15 @@ def test_admin_migrate(cli_call, mock_func_factory): def test_admin_clean_deleted_relations(cli_call, mock_func_factory): mock = mock_func_factory(nominatim.tools.admin, 'clean_deleted_relations') - assert cli_call('admin', '--clean-deleted') == 0 + assert cli_call('admin', '--clean-deleted', '--age', '1 month') == 0 assert mock.called == 1 +def test_admin_clean_deleted_relations_no_age(cli_call, mock_func_factory): + mock = mock_func_factory(nominatim.tools.admin, 'clean_deleted_relations') + + with pytest.raises(SystemExit): + cli_call('admin', '--clean-deleted') + class TestCliAdminWithDb: @pytest.fixture(autouse=True) diff --git a/test/python/tools/test_admin.py b/test/python/tools/test_admin.py index 3ce3c8f4..ae5944af 100644 --- a/test/python/tools/test_admin.py +++ b/test/python/tools/test_admin.py @@ -91,8 +91,8 @@ class TestAdminCleanDeleted: (175, 'R', 'landcover', 'grass'))) temp_db_cursor.execute("""INSERT INTO placex (place_id, osm_id, osm_type, class, type, indexed_date, indexed_status) VALUES(1, 100, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), - (2, 145, 'N', 'boundary', 'administrative', current_date - INTERVAL '1 month', 1), - (3, 175, 'R', 'landcover', 'grass', current_date - INTERVAL '1 month', 1)""") + (2, 145, 'N', 'boundary', 'administrative', current_date - INTERVAL '3 month', 1), + (3, 175, 'R', 'landcover', 'grass', current_date - INTERVAL '3 months', 1)""") # set up tables and triggers for utils function table_factory('place_to_be_deleted', """osm_id BIGINT, @@ -126,17 +126,19 @@ class TestAdminCleanDeleted: assert self.temp_db_cursor.table_rows('import_polygon_delete') == 3 - def test_admin_clean_deleted_no_age(self): - with pytest.raises(UsageError): - admin.clean_deleted_relations(self.project_env) - - @pytest.mark.parametrize('test_age', ['T week', '1 welk', 'P1E']) def test_admin_clean_deleted_bad_age(self, test_age): with pytest.raises(UsageError): admin.clean_deleted_relations(self.project_env, age = test_age) + def test_admin_clean_deleted_partial(self): + admin.clean_deleted_relations(self.project_env, age = '2 months') + assert self.temp_db_cursor.row_set('SELECT osm_id, osm_type, class, type, indexed_status FROM placex') == {(100, 'N', 'boundary', 'administrative', 1), + (145, 'N', 'boundary', 'administrative', 100), + (175, 'R', 'landcover', 'grass', 100)} + assert self.temp_db_cursor.table_rows('import_polygon_delete') == 1 + @pytest.mark.parametrize('test_age', ['1 week', 'P3D', '5 hours']) def test_admin_clean_deleted(self, test_age): admin.clean_deleted_relations(self.project_env, age = test_age) From 650fbc25637c9ad0893594a6c1bc8dc25171143d Mon Sep 17 00:00:00 2001 From: lujoh Date: Wed, 18 Oct 2023 02:03:17 -0400 Subject: [PATCH 4/5] added --clean deleted command to the documentation --- docs/admin/Maintenance.md | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/docs/admin/Maintenance.md b/docs/admin/Maintenance.md index 1ee313a9..758ede65 100644 --- a/docs/admin/Maintenance.md +++ b/docs/admin/Maintenance.md @@ -60,16 +60,14 @@ to finish the recomputation. ## Removing large deleted objects +Command: `nominatim admin --clean-deleted --age ` + Nominatim refuses to delete very large areas because often these deletions are accidental and are reverted within hours. Instead the deletions are logged in the `import_polygon_delete` table and left to the administrator to clean up. -There is currently no command to do that. You can use the following SQL -query to force a deletion on all objects that have been deleted more than -a certain timespan ago (here: 1 month): +To run this command you will need to pass a PostgreSQL time interval to the age +parameter. For example to delete any objects that have been deleted more than a +month ago you would run: +`nominatim admin --clean-deleted --age '1 month'` -```sql -SELECT place_force_delete(p.place_id) FROM import_polygon_delete d, placex p -WHERE p.osm_type = d.osm_type and p.osm_id = d.osm_id - and age(p.indexed_date) > '1 month'::interval -``` From 418f381b492cc9c0f3ede008e72e1d2eb1a611a8 Mon Sep 17 00:00:00 2001 From: lujoh Date: Fri, 20 Oct 2023 15:31:55 -0400 Subject: [PATCH 5/5] made age a required argument for the -clean-deleted command --- docs/admin/Maintenance.md | 9 ++++----- nominatim/clicmd/admin.py | 8 ++------ nominatim/clicmd/args.py | 3 +-- test/python/cli/test_cmd_admin.py | 5 ++--- 4 files changed, 9 insertions(+), 16 deletions(-) diff --git a/docs/admin/Maintenance.md b/docs/admin/Maintenance.md index 758ede65..325e6f8f 100644 --- a/docs/admin/Maintenance.md +++ b/docs/admin/Maintenance.md @@ -60,14 +60,13 @@ to finish the recomputation. ## Removing large deleted objects -Command: `nominatim admin --clean-deleted --age ` +Command: `nominatim admin --clean-deleted ` Nominatim refuses to delete very large areas because often these deletions are accidental and are reverted within hours. Instead the deletions are logged in the `import_polygon_delete` table and left to the administrator to clean up. -To run this command you will need to pass a PostgreSQL time interval to the age -parameter. For example to delete any objects that have been deleted more than a -month ago you would run: -`nominatim admin --clean-deleted --age '1 month'` +To run this command you will need to pass a PostgreSQL time interval. For example to +delete any objects that have been deleted more than a month ago you would run: +`nominatim admin --clean-deleted '1 month'` diff --git a/nominatim/clicmd/admin.py b/nominatim/clicmd/admin.py index 9a7b92e6..d1ce5ccc 100644 --- a/nominatim/clicmd/admin.py +++ b/nominatim/clicmd/admin.py @@ -42,7 +42,7 @@ class AdminFuncs: help='Print performance analysis of the indexing process') objs.add_argument('--collect-os-info', action="store_true", help="Generate a report about the host system information") - objs.add_argument('--clean-deleted', action='store_true', + objs.add_argument('--clean-deleted', action='store', metavar='AGE', help='Clean up deleted relations') group = parser.add_argument_group('Arguments for cache warming') group.add_argument('--search-only', action='store_const', dest='target', @@ -58,8 +58,6 @@ class AdminFuncs: mgroup.add_argument('--place-id', type=int, help='Analyse indexing of the given Nominatim object') group = parser.add_argument_group('Arguments for cleaning deleted') - group.add_argument('--age', type=str, - help='Delete relations older than the given PostgreSQL time interval') def run(self, args: NominatimArgs) -> int: @@ -90,11 +88,9 @@ class AdminFuncs: return 0 if args.clean_deleted: - if not args.age: - self.parser.error('Age is required for --clean-deleted command') LOG.warning('Cleaning up deleted relations') from ..tools import admin - admin.clean_deleted_relations(args.config, age=args.age) + admin.clean_deleted_relations(args.config, age=args.clean_deleted) return 0 return 1 diff --git a/nominatim/clicmd/args.py b/nominatim/clicmd/args.py index 5b4beb40..e632e4c7 100644 --- a/nominatim/clicmd/args.py +++ b/nominatim/clicmd/args.py @@ -72,12 +72,11 @@ class NominatimArgs: check_database: bool migrate: bool collect_os_info: bool - clean_deleted: bool + clean_deleted: str analyse_indexing: bool target: Optional[str] osm_id: Optional[str] place_id: Optional[int] - age: str # Arguments to 'import' osm_file: List[str] diff --git a/test/python/cli/test_cmd_admin.py b/test/python/cli/test_cmd_admin.py index 0fe6e327..45104ea6 100644 --- a/test/python/cli/test_cmd_admin.py +++ b/test/python/cli/test_cmd_admin.py @@ -36,14 +36,13 @@ def test_admin_migrate(cli_call, mock_func_factory): def test_admin_clean_deleted_relations(cli_call, mock_func_factory): mock = mock_func_factory(nominatim.tools.admin, 'clean_deleted_relations') - assert cli_call('admin', '--clean-deleted', '--age', '1 month') == 0 + assert cli_call('admin', '--clean-deleted', '1 month') == 0 assert mock.called == 1 def test_admin_clean_deleted_relations_no_age(cli_call, mock_func_factory): mock = mock_func_factory(nominatim.tools.admin, 'clean_deleted_relations') - with pytest.raises(SystemExit): - cli_call('admin', '--clean-deleted') + assert cli_call('admin', '--clean-deleted') == 1 class TestCliAdminWithDb: