Future of db/functions?

[mmd-osm]

Now that we have migrated from osmosis to osmdbt for replication diffs, and quadtile calculation resides in its own Gem, we could think about removing db/functions altogether, like shown in https://github.com/mmd-osm/openstreetmap-website/commit/d5c3828db15f5aa1e0fd90baef486e8966fdfaa4

Also, we started a discussion on this topic in https://github.com/openstreetmap/openstreetmap-website/issues/3081#issuecomment-773210432, which can continue here now.

Before creating a pull request for this topic, I'd like to get some early feedback, if this really makes sense.

Some points to consider:

• People might still want to try out osmosis replication in their dev environment. Is this in scope for this repo?
  • Alternative option to install sql function via https://github.com/openstreetmap/osmosis/blob/master/package/script/contrib/apidb_0.6_osmosis_xid_indexing.sql instead of using the respective db function in this repo.
  • There are some CREATE INDEX statements in there that might no longer work on Postgres > 9.5, i.e. that file might not be suitable as is today.
• Impact on Rails migrations:
  • do we need to clean up old db functions? This also applies to the already deleted maptile_for_point function.
  • do we still need those old db functions to run old migrations? (topic of https://github.com/openstreetmap/openstreetmap-website/issues/3081#issuecomment-773210432)

[tomhughes]

Please don't remove anything yet - the production database still has those functions for now.

[gravitystorm]

Before creating a pull request for this topic, I'd like to get some early feedback, if this really makes sense.

I'm always in favour of cleanup, and removing complexity in particular :smile:

* People might still want to try out osmosis replication in their dev environment. Is this in scope for this repo?

We should push the concept of setting up replication out of this repo. I think the best approach is for osmosis to ship the functions necessary for osmosis-based replication, and osmdbt to ship whatever it needs too. We should update CONFIGURE.md to indicate there are two options to consider (osmosis and osmdbt) depending on your PostgreSQL verison and point people to the right place, but otherwise remove ourselves from that.

* do we still need those old db functions to run old migrations?

This is the core of the discussion, I think. We need to keep the db functions for as long as they are used in the old migrations.

We could refactor the old migrations, to remove the functions. But this raises the question of what the old migrations are there for? Two possibilities:

• They a there to help people who have a (really old) database with data in it. So we need to keep the old migrations and the functions too.
• They are there to help people who have an empty database. In which case we don't need the old migrations at all, since it's much easier (and faster) to load the structure.sql than to run through 109 migrations.

I believe we should just ditch the old migrations, say every migration over 2 years old. If someone needs them (if they have a really old database dump, or they only update their code every 3 years) then they are in source control anyway, just like our old unused code is. We can keep the reasonably new migrations to help other developers and deployments who don't update every month.

The other alternative is to keep the old migrations, but continue to refactor them over the years, either for things like this, or rubocop, or to work with new database systems, or whatever. But I think this is just makework, like keeping old code in comments or like maintaining amf_controller even after we remove it from the API. The code in migrations is old, it has done its job, it's their in the git history if anyone needs it.

I think removing the old migrations will mean we need some tweaks to CI and documentation, e.g. to load empty databases from the structure.sql (and use commands like db:structure:load) instead of migrations. There is also the 'double check' that we do in CI where we test that the sum of the migrations is equal to the structure.sql, so we need an alternative (e.g. a check that old_structure.sql + migrations == structure.sql).

[tordans]

I believe we should just ditch the old migrations, say every migration over 2 years old.

What we do at betterplace.org is: We run https://github.com/jalkoby/squasher about once a year or so which gives us a single migration file which creates the DB in just the way that it would look like after running all single migrations. This still allows to run migrations and end up with a correct DB setup.

I did in fact try out to use squasher around last December/January for this project but ran into errors that looked like they are PostgreSQL related … or maybe had something to do with special indexes or primay keys(?) (It might be the same as the composite_primary_keys gem you guys talk about in the Rails6.1 branch).

[tomhughes]

We have various extra migration features monkey patched into rails (by https://github.com/openstreetmap/openstreetmap-website/blob/master/config/initializers/migrate.rb) which are probably confusing it.

[mmd-osm]

I believe tile_for_point is the only function that has been used for back-filling data as part of a migration. A "grep" shows 3 occurrences of that function, all of them are older than 3 years.

$ grep -rin "tile_for_point"
006_tile_nodes.rb:16:             tile_for_point(CAST(ROUND(latitude * 10000000) AS INTEGER),
005_tile_tracepoints.rb:12:      Tracepoint.update_all("latitude = latitude * 10, longitude = longitude * 10, tile = tile_for_point(latitude * 10, longitude * 10)")
20180204153242_tile_users.rb:10:      User.update_all("home_tile = tile_for_point(cast(round(home_lat * #{GeoRecord::SCALE}) as integer), cast(round(home_lon * #{GeoRecord::SCALE}) as integer))")

[gravitystorm]

I did in fact try out to use squasher around last December/January for this project but ran into errors that looked like they are PostgreSQL related … or maybe had something to do with special indexes or primay keys(?) (It might be the same as the composite_primary_keys gem you guys talk about in the Rails6.1 branch).

I got squasher working today - you can see the results at https://github.com/gravitystorm/openstreetmap-website/commit/9890f754c012266617000c1fca025a93bd432ac4 . The trick was to add a create_extension "btree_gist" to the first migration, since squasher creates its own database automatically, and that extension is needed for some index stuff later in the 028 migration. The primary key and enum monkey patching doesn't seem to cause any problems.

So squasher seems like a reasonable compromise between removing all the old migrations, but without having to change any workflows, since rake db:migrate will still work on an empty database. I'm interested to hear what other people think?

Additional points to consider:

• Should we include the squasher 'cleanup' migration, which removes the no-longer-required entries from the schema_migrations table?
• Need to update rubocop config, and check that the output schema matches the migrations (in case there's an edge case in the sql format dumping) since there are test failures.
• I'm also interested if we can move away from SQL format db dumps, which are quite verbose files. There's a pg_enum gem that could replace our custom enum code, and that also knows how to dump the enum information back into schema.rb files. If we're also removing the functions, is there anything left that needs SQL format dumps?
• If we decide all this is a bad idea and abandon this work, we should at least remember to add create_extension "btree_gist" to the appropriate migration and simplify our install instructions :smile:.

[tomhughes]

The other customisations we have are the ability to create/alter/drop primary keys which I don't think rails can do other than as part of creating a table and then only for a single field (unless CPK changes that) and support for :columns when creating an index to specify exotic indexes with expressions.

[mmd-osm]

I'm also interested if we can move away from SQL format db dumps, which are quite verbose files.

I’m aware of at least 3 projects that depend on that file being available for some very occasional updates: osmdbt, cgimap and osmosis. Those would use their own copy of structure.sql for setting up a unit test database. If there’s some way to create the SQL structure dump on demand, that would still be fine, though.

[mmd-osm]

First bit addressed in https://github.com/openstreetmap/openstreetmap-website/commit/81c3ebe03c229c980908502f3735cdf1636dd5cc