From a173a5c4987e3a9569ee3e10bbab2bd5cf70dbc8 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Sun, 5 Jul 2026 17:53:19 -0400 Subject: [PATCH] Index for public map building --- .../0035_public_repeater_map_index.sql | 22 +++ internal/store/org_repeaters_test.go | 177 ++++++++++++++++++ 2 files changed, 199 insertions(+) create mode 100644 internal/store/migrations/0035_public_repeater_map_index.sql create mode 100644 internal/store/org_repeaters_test.go diff --git a/internal/store/migrations/0035_public_repeater_map_index.sql b/internal/store/migrations/0035_public_repeater_map_index.sql new file mode 100644 index 0000000..28505ee --- /dev/null +++ b/internal/store/migrations/0035_public_repeater_map_index.sql @@ -0,0 +1,22 @@ +-- +goose Up +-- The public org map (ListPublicRepeaterPoints) and public repeater list +-- (ListPublicRepeaters/…Page) join org_members → repeaters(owner_id) and then +-- filter r.show_on_public_org (and, for the map, latitude/longitude NOT NULL). +-- The only pre-existing repeaters index is repeaters_owner_id_idx (all rows), so +-- that filter fell back to a scan of the owner's repeaters. These are served on +-- the unauthenticated root host, so they run on every anonymous visit. +-- +-- A partial index on owner_id restricted to the map predicate lets the nested-loop +-- join seek straight to an owner's public, located repeaters — the join key stays +-- owner_id (so the planner can still use it), while the WHERE prunes the index to +-- only the rows the map can show. It also shrinks with the eligible set rather than +-- the whole table. +-- +-- Note for a future *instance-wide* map (all orgs, no org_members join): that path +-- would seek by geography, not owner_id, and should get its own (latitude, +-- longitude) index — this one is for the per-org, owner-joined queries. +CREATE INDEX repeaters_public_map_idx ON repeaters (owner_id) + WHERE show_on_public_org AND latitude IS NOT NULL AND longitude IS NOT NULL; + +-- +goose Down +DROP INDEX IF EXISTS repeaters_public_map_idx; diff --git a/internal/store/org_repeaters_test.go b/internal/store/org_repeaters_test.go new file mode 100644 index 0000000..807873f --- /dev/null +++ b/internal/store/org_repeaters_test.go @@ -0,0 +1,177 @@ +package store + +import ( + "strings" + "testing" + + "github.com/jackc/pgx/v5" +) + +// TestListPublicRepeaterPoints pins the map query's predicate: a repeater is +// plotted iff its owner is a member of the org, it opted into show_on_public_org, +// it has coordinates, and it isn't excluded from the org. This is the set the +// unauthenticated org map (and the repeaters_public_map_idx partial index) covers. +func TestListPublicRepeaterPoints(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + + owner, err := st.CreateUser(ctx, "owner", "") + if err != nil { + t.Fatalf("create owner: %v", err) + } + org, err := st.CreateOrg(ctx, "Org", owner.ID) // creator becomes a member + if err != nil { + t.Fatalf("create org: %v", err) + } + + // mk creates a repeater owned by owner with the given public/location state and + // returns its id. keyChar keeps the public key unique per repeater. + mk := func(name string, keyChar byte, public bool, located bool) int64 { + t.Helper() + r, err := st.CreateRepeater(ctx, &Repeater{ + OwnerID: owner.ID, Name: name, PublicKeyHex: strings.Repeat(string(keyChar), 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + ShowOnPublicOrg: public, + }) + if err != nil { + t.Fatalf("create repeater %s: %v", name, err) + } + if located { + if err := st.SetRepeaterLocation(ctx, r.ID, 40.0, -75.0); err != nil { + t.Fatalf("set location %s: %v", name, err) + } + } + return r.ID + } + + mk("shown", 'a', true, true) // the only one that should appear + mk("private", 'b', false, true) // opted out of public + mk("no-location", 'c', true, false) // public but unlocated + excluded := mk("excluded", 'd', true, true) + + if err := st.SetRepeaterOrgExcluded(ctx, org.ID, excluded, true); err != nil { + t.Fatalf("exclude: %v", err) + } + + // A public, located repeater owned by a non-member must not appear. + stranger, err := st.CreateUser(ctx, "stranger", "") + if err != nil { + t.Fatalf("create stranger: %v", err) + } + sr, err := st.CreateRepeater(ctx, &Repeater{ + OwnerID: stranger.ID, Name: "stranger-rep", PublicKeyHex: strings.Repeat("e", 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, ShowOnPublicOrg: true, + }) + if err != nil { + t.Fatalf("create stranger repeater: %v", err) + } + if err := st.SetRepeaterLocation(ctx, sr.ID, 41.0, -76.0); err != nil { + t.Fatalf("set stranger location: %v", err) + } + + points, err := st.ListPublicRepeaterPoints(ctx, org.ID) + if err != nil { + t.Fatalf("ListPublicRepeaterPoints: %v", err) + } + if len(points) != 1 { + t.Fatalf("got %d points, want 1: %+v", len(points), points) + } + if points[0].Name != "shown" || points[0].Lat != 40.0 || points[0].Lon != -75.0 { + t.Fatalf("unexpected point %+v (want shown @ 40,-75)", points[0]) + } +} + +// TestPublicRepeaterMapUsesPartialIndex proves migration 0035's partial index is +// the access path the map query takes: with a member owning many repeaters of +// which only a few are public+located, the planner must reach the eligible rows +// through repeaters_public_map_idx rather than scanning all of the owner's rows. +// This enforces the performance characteristic (CLAUDE.md: perf claims need proof), +// so a future query change that stops matching the index fails here. +func TestPublicRepeaterMapUsesPartialIndex(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + + owner, err := st.CreateUser(ctx, "owner", "") + if err != nil { + t.Fatalf("create owner: %v", err) + } + org, err := st.CreateOrg(ctx, "Org", owner.ID) + if err != nil { + t.Fatalf("create org: %v", err) + } + + // Bulk-insert many private, unlocated repeaters (not in the partial index) plus + // a handful of public+located ones (in it). Inserted directly for speed; hex key + // is derived from n to stay unique and 64 chars. + const total, eligible = 2000, 5 + for n := 0; n < total; n++ { + public := n < eligible + _, err := st.pool.Exec(ctx, ` + INSERT INTO repeaters (public_id, owner_id, name, public_key_hex, + radio_freq_hz, radio_bw_hz, radio_sf, radio_cr, + show_on_public_org, latitude, longitude) + VALUES ($1, $2, $3, $4, 1, 1, 11, 5, $5, $6, $7)`, + "pub"+itoa(n), owner.ID, "R"+itoa(n), + leftPad(itoa(n), '0', 64), public, + nullableFloat(public, 40.0), nullableFloat(public, -75.0)) + if err != nil { + t.Fatalf("insert repeater %d: %v", n, err) + } + } + if _, err := st.pool.Exec(ctx, `ANALYZE repeaters`); err != nil { + t.Fatalf("analyze: %v", err) + } + + // EXPLAIN the exact predicate ListPublicRepeaterPoints uses (kept in sync with + // that query) and require the partial index in the chosen plan. EXPLAIN returns + // one row per plan line, so collect them all. + rows, err := st.pool.Query(ctx, ` + EXPLAIN (FORMAT TEXT) + SELECT r.name, r.latitude, r.longitude + FROM repeaters r + JOIN org_members om ON om.org_id = $1 AND om.user_id = r.owner_id + WHERE r.show_on_public_org + AND r.latitude IS NOT NULL AND r.longitude IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM org_repeater_excludes e + WHERE e.org_id = $1 AND e.repeater_id = r.id)`, org.ID) + if err != nil { + t.Fatalf("explain: %v", err) + } + lines, err := collectRows(rows, func(r pgx.Row) (string, error) { + var line string + return line, r.Scan(&line) + }) + if err != nil { + t.Fatalf("scan plan: %v", err) + } + plan := strings.Join(lines, "\n") + if !strings.Contains(plan, "repeaters_public_map_idx") { + t.Fatalf("map query did not use repeaters_public_map_idx; plan:\n%s", plan) + } +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + var b []byte + for n > 0 { + b = append([]byte{byte('0' + n%10)}, b...) + n /= 10 + } + return string(b) +} + +func leftPad(s string, pad byte, width int) string { + if len(s) >= width { + return s[:width] + } + return strings.Repeat(string(pad), width-len(s)) + s +} + +func nullableFloat(present bool, v float64) *float64 { + if !present { + return nil + } + return &v +}