[openstreetmap/openstreetmap-website] Move GPS tables to a separate database (PR #7111)

[Andy Allan]

@gravitystorm requested changes on this pull request.

In general I'm neutral on this idea. I've read the operations task and I haven't seen convincing evidence that this is the best way to solve the operational problems. From a codebase point of view, this adds significant complexity to all deployments, so we need to make really sure that it's worth doing.

For this PR, in addition to my inline notes, I would say:
* There's no task included to migrate the data from the old database to the new one. This is critical, particularly for non-OSMF deployments that you are not personally managing.
* There's no migration strategy. Do we expect all the traces to disappear from the site, then slowly reappear as the migration progresses? Or is there going to be a (several-day?) outage during the migration?
* Or should the general strategy be more like the ["renaming a table" strategy](https://github.com/ankane/strong_migrations#renaming-a-table) documented in strong_migrations?
* When this PR runs, the site will instantly use the new database for new uploads. This will then immediately conflict with any data that is being migrated from the old database to the new (e.g. primary key reuse).

> @@ -12,6 +12,10 @@ class ApplicationController < ActionController::Base
 
   rescue_from RailsParam::InvalidParameterError, :with => :invalid_parameter
 
+  rescue_from ActiveRecord::ConnectionNotEstablished,
+              ActiveRecord::DatabaseConnectionError,
+              ActiveRecord::NoDatabaseError, :with => :gps_database_unavailable
+

This isn't correct, those errors could be for other reasons

> @@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+class GpsRecord < ApplicationRecord

We should consider the naming of this abstract class (and the database name) more carefully.

When OSM was created, "GPS" and satellite positioning was more or less interchangeable. However, time has moved on, and the American GPS system is only one of 4 different GNSS systems.

GNSS is, however, another acronym, and not very self-explanatory - nor is it in common use.

Was the decision to call the database "gps" and the model "GpsRecord" deliberate? Or would something involving "trace" be more consistent with e.g. the model naming?

> @@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+# Drop the GPS tables (gps_points, gpx_file_tags, gpx_files) from the main database.
+# These tables now live in a separate GPS database (see GpsRecord model).
+class DropGpsTablesFromMainDb < ActiveRecord::Migration[8.1]

If anyone runs this migration, they will lose all of their traces in their development database.

It's also something that we couldn't merge as-is, since anyone deploying this PR would lose all the trace data from their production database.

> @@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+class CreateGpsTables < ActiveRecord::Migration[8.1]
+  def change
+    create_enum :gpx_visibility_enum, %w[private public trackable identifiable]
+
+    create_table :gpx_files, :id => :bigint do |t|

If we're going to create a new database, then I think this is an opportunity to fix the table naming, to match the model names.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7111#pullrequestreview-4371472265
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/7111/review/4371472265 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260527/64db014d/attachment.htm>