From 377a9df3b3db08b7eb224124e31e22bca1f4aa29 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Sun, 5 Jul 2026 12:46:36 -0400 Subject: [PATCH] Optimize orgs list queries --- internal/store/migrations/0033_org_counts.sql | 141 ++++++++++++++++ internal/store/org_counts_test.go | 153 ++++++++++++++++++ internal/store/orgs.go | 52 +++--- 3 files changed, 315 insertions(+), 31 deletions(-) create mode 100644 internal/store/migrations/0033_org_counts.sql create mode 100644 internal/store/org_counts_test.go diff --git a/internal/store/migrations/0033_org_counts.sql b/internal/store/migrations/0033_org_counts.sql new file mode 100644 index 0000000..025274b --- /dev/null +++ b/internal/store/migrations/0033_org_counts.sql @@ -0,0 +1,141 @@ +-- +goose Up +-- Denormalize the public org directory's member/repeater counts onto the +-- organizations row so the directory can sort and keyset-seek on real, indexed +-- columns instead of computing a correlated count subquery per org on every +-- (unauthenticated) page load. Kept exact by triggers that recompute the +-- affected org from source of truth on any membership/repeater/exclude change. +-- +-- member_count = |org_members for the org| +-- repeater_count = repeaters owned by a member of the org, minus per-org +-- excludes (org_repeater_excludes) — the same definition the +-- old subquery and OrgCounts use. +ALTER TABLE organizations ADD COLUMN member_count INT NOT NULL DEFAULT 0; +ALTER TABLE organizations ADD COLUMN repeater_count INT NOT NULL DEFAULT 0; + +-- Recompute both counts for one org from the source tables. Recomputing (rather +-- than applying ±1 deltas) is immune to trigger ordering and to the ON DELETE +-- CASCADE on org_repeater_excludes.repeater_id: it always reads the current +-- state. A no-op when the org no longer exists (e.g. recompute racing an org +-- delete cascade) — the UPDATE simply matches no rows. +-- +goose StatementBegin +CREATE OR REPLACE FUNCTION org_recompute_counts(p_org_id BIGINT) RETURNS void AS $$ + UPDATE organizations o SET + member_count = (SELECT count(*) FROM org_members m WHERE m.org_id = o.id), + repeater_count = ( + SELECT count(*) FROM repeaters r + JOIN org_members om ON om.org_id = o.id AND om.user_id = r.owner_id + WHERE NOT EXISTS ( + SELECT 1 FROM org_repeater_excludes e + WHERE e.org_id = o.id AND e.repeater_id = r.id)) + WHERE o.id = p_org_id; +$$ LANGUAGE sql; +-- +goose StatementEnd + +-- A membership change affects exactly one org. +-- +goose StatementBegin +CREATE OR REPLACE FUNCTION org_members_recount() RETURNS trigger AS $$ +BEGIN + IF TG_OP = 'DELETE' THEN + PERFORM org_recompute_counts(OLD.org_id); + RETURN OLD; + END IF; + PERFORM org_recompute_counts(NEW.org_id); + RETURN NEW; +END; +$$ LANGUAGE plpgsql; +-- +goose StatementEnd + +-- An exclude change affects the repeater_count of exactly one org. +-- +goose StatementBegin +CREATE OR REPLACE FUNCTION org_excludes_recount() RETURNS trigger AS $$ +BEGIN + IF TG_OP = 'DELETE' THEN + PERFORM org_recompute_counts(OLD.org_id); + RETURN OLD; + END IF; + PERFORM org_recompute_counts(NEW.org_id); + RETURN NEW; +END; +$$ LANGUAGE plpgsql; +-- +goose StatementEnd + +-- A repeater insert/delete/owner-change affects every org its owner(s) belong +-- to. On DELETE the row is already gone when this AFTER trigger runs, so the +-- recompute correctly excludes it; on an owner change both owners' orgs are +-- refreshed. +-- +goose StatementBegin +CREATE OR REPLACE FUNCTION repeaters_recount() RETURNS trigger AS $$ +DECLARE + oid BIGINT; +BEGIN + IF TG_OP <> 'INSERT' THEN + FOR oid IN SELECT org_id FROM org_members WHERE user_id = OLD.owner_id LOOP + PERFORM org_recompute_counts(oid); + END LOOP; + END IF; + IF TG_OP <> 'DELETE' THEN + FOR oid IN SELECT org_id FROM org_members WHERE user_id = NEW.owner_id LOOP + PERFORM org_recompute_counts(oid); + END LOOP; + END IF; + IF TG_OP = 'DELETE' THEN RETURN OLD; END IF; + RETURN NEW; +END; +$$ LANGUAGE plpgsql; +-- +goose StatementEnd + +CREATE TRIGGER org_members_recount_trg + AFTER INSERT OR DELETE ON org_members + FOR EACH ROW EXECUTE FUNCTION org_members_recount(); + +CREATE TRIGGER org_excludes_recount_trg + AFTER INSERT OR DELETE ON org_repeater_excludes + FOR EACH ROW EXECUTE FUNCTION org_excludes_recount(); + +-- UPDATE only matters when the owner changes (name/radio edits don't move a +-- repeater between orgs), so scope the UPDATE event to owner_id. +CREATE TRIGGER repeaters_recount_trg + AFTER INSERT OR DELETE OR UPDATE OF owner_id ON repeaters + FOR EACH ROW EXECUTE FUNCTION repeaters_recount(); + +-- Backfill existing rows now that the triggers are in place. +UPDATE organizations o SET + member_count = (SELECT count(*) FROM org_members m WHERE m.org_id = o.id), + repeater_count = ( + SELECT count(*) FROM repeaters r + JOIN org_members om ON om.org_id = o.id AND om.user_id = r.owner_id + WHERE NOT EXISTS ( + SELECT 1 FROM org_repeater_excludes e + WHERE e.org_id = o.id AND e.repeater_id = r.id)); + +-- Sort/seek indexes for the directory orderings. (Name ASC is already served by +-- organizations_name_id_idx.) Each matches an ORDER BY (col, id) DESC + its +-- keyset row comparison. +CREATE INDEX organizations_member_count_id_idx ON organizations (member_count DESC, id DESC); +CREATE INDEX organizations_repeater_count_id_idx ON organizations (repeater_count DESC, id DESC); +CREATE INDEX organizations_created_at_id_idx ON organizations (created_at DESC, id DESC); + +-- Trigram indexes so the directory's substring search (name/description/region +-- ILIKE '%q%') stops sequentially scanning the whole table. +CREATE EXTENSION IF NOT EXISTS pg_trgm; +CREATE INDEX organizations_name_trgm_idx ON organizations USING gin (name gin_trgm_ops); +CREATE INDEX organizations_description_trgm_idx ON organizations USING gin (description gin_trgm_ops); +CREATE INDEX organizations_region_trgm_idx ON organizations USING gin (region gin_trgm_ops); + +-- +goose Down +DROP INDEX IF EXISTS organizations_region_trgm_idx; +DROP INDEX IF EXISTS organizations_description_trgm_idx; +DROP INDEX IF EXISTS organizations_name_trgm_idx; +DROP EXTENSION IF EXISTS pg_trgm; +DROP INDEX IF EXISTS organizations_created_at_id_idx; +DROP INDEX IF EXISTS organizations_repeater_count_id_idx; +DROP INDEX IF EXISTS organizations_member_count_id_idx; +DROP TRIGGER IF EXISTS repeaters_recount_trg ON repeaters; +DROP TRIGGER IF EXISTS org_excludes_recount_trg ON org_repeater_excludes; +DROP TRIGGER IF EXISTS org_members_recount_trg ON org_members; +DROP FUNCTION IF EXISTS repeaters_recount(); +DROP FUNCTION IF EXISTS org_excludes_recount(); +DROP FUNCTION IF EXISTS org_members_recount(); +DROP FUNCTION IF EXISTS org_recompute_counts(BIGINT); +ALTER TABLE organizations DROP COLUMN repeater_count; +ALTER TABLE organizations DROP COLUMN member_count; diff --git a/internal/store/org_counts_test.go b/internal/store/org_counts_test.go new file mode 100644 index 0000000..29b85e9 --- /dev/null +++ b/internal/store/org_counts_test.go @@ -0,0 +1,153 @@ +package store + +import ( + "context" + "strings" + "testing" +) + +// orgCountHarness bundles the store + a consistency assertion for the +// trigger-maintained directory counts (migration 0033). The triggers are +// correct iff the denormalized organizations.member_count/repeater_count always +// equal OrgCounts, which recomputes from the source tables. +type orgCountHarness struct { + t *testing.T + st *Store + ctx context.Context +} + +func (h *orgCountHarness) assert(orgID int64, label string, wantMembers, wantReps int) { + h.t.Helper() + var dm, dr int + if err := h.st.pool.QueryRow(h.ctx, + `SELECT member_count, repeater_count FROM organizations WHERE id=$1`, orgID).Scan(&dm, &dr); err != nil { + h.t.Fatalf("%s: read denormalized counts: %v", label, err) + } + lm, lr, err := h.st.OrgCounts(h.ctx, orgID) + if err != nil { + h.t.Fatalf("%s: OrgCounts: %v", label, err) + } + if dm != lm || dr != lr { + h.t.Fatalf("%s: denormalized (m=%d r=%d) != recomputed (m=%d r=%d)", label, dm, dr, lm, lr) + } + if dm != wantMembers || dr != wantReps { + h.t.Fatalf("%s: counts m=%d r=%d, want m=%d r=%d", label, dm, dr, wantMembers, wantReps) + } +} + +func (h *orgCountHarness) user(name string) int64 { + h.t.Helper() + u, err := h.st.CreateUser(h.ctx, name, "") + if err != nil { + h.t.Fatalf("create user %s: %v", name, err) + } + return u.ID +} + +func (h *orgCountHarness) repeater(owner int64, keyChar byte) int64 { + h.t.Helper() + r, err := h.st.CreateRepeater(h.ctx, &Repeater{ + OwnerID: owner, Name: "R", PublicKeyHex: strings.Repeat(string(keyChar), 64), + RadioFreqHz: 1, RadioBwHz: 1, RadioSF: 11, RadioCR: 5, + }) + if err != nil { + h.t.Fatalf("create repeater: %v", err) + } + return r.ID +} + +// TestOrgCountTriggers walks a repeater/membership/exclude lifecycle and checks +// the denormalized counts stay exact and in sync with the live computation after +// every mutation. +func TestOrgCountTriggers(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + h := &orgCountHarness{t, st, ctx} + + owner := h.user("owner") + org, err := st.CreateOrg(ctx, "Org", owner) // creator becomes an admin member + if err != nil { + t.Fatalf("create org: %v", err) + } + h.assert(org.ID, "after create", 1, 0) + + // The owner's repeater participates automatically (no opt-out). + repA := h.repeater(owner, 'a') + h.assert(org.ID, "owner adds repeater", 1, 1) + + m2 := h.user("m2") + if err := st.AddOrgMember(ctx, org.ID, m2, "member"); err != nil { + t.Fatalf("add member: %v", err) + } + h.assert(org.ID, "second member joins", 2, 1) + + h.repeater(m2, 'b') + repC := h.repeater(m2, 'c') + h.assert(org.ID, "member adds two repeaters", 2, 3) + + if err := st.SetRepeaterOrgExcluded(ctx, org.ID, repC, true); err != nil { + t.Fatalf("exclude: %v", err) + } + h.assert(org.ID, "exclude one repeater", 2, 2) + + if err := st.SetRepeaterOrgExcluded(ctx, org.ID, repC, false); err != nil { + t.Fatalf("re-include: %v", err) + } + h.assert(org.ID, "re-include repeater", 2, 3) + + if err := st.DeleteRepeaterOwned(ctx, owner, repA); err != nil { + t.Fatalf("delete repeater: %v", err) + } + h.assert(org.ID, "delete owner repeater", 2, 2) + + // Member leaves: member_count drops, and both of that member's repeaters leave + // the count (owner no longer a member) → repeater_count 0. + if err := st.RemoveOrgMember(ctx, org.ID, m2); err != nil { + t.Fatalf("remove member: %v", err) + } + h.assert(org.ID, "member leaves", 1, 0) +} + +// TestOrgCountCascades covers the DB-level cascades that have no store method: +// deleting a user (cascades their org_members + repeaters) must keep counts +// consistent, and deleting an org (cascades members/excludes, firing recompute +// on the vanishing org) must not error. +func TestOrgCountCascades(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + h := &orgCountHarness{t, st, ctx} + + owner := h.user("owner") + org, err := st.CreateOrg(ctx, "Org", owner) + if err != nil { + t.Fatalf("create org: %v", err) + } + h.repeater(owner, 'a') + + member := h.user("member") + if err := st.AddOrgMember(ctx, org.ID, member, "member"); err != nil { + t.Fatalf("add member: %v", err) + } + h.repeater(member, 'b') + h.assert(org.ID, "two members, two repeaters", 2, 2) + + // Deleting the member cascades to org_members (ON DELETE CASCADE) and their + // repeaters (owner_id ON DELETE CASCADE); both trigger recomputes. + if _, err := st.pool.Exec(ctx, `DELETE FROM users WHERE id=$1`, member); err != nil { + t.Fatalf("delete user: %v", err) + } + h.assert(org.ID, "member user deleted", 1, 1) + + // Deleting the org cascades members/excludes; the recompute targets a row that + // no longer exists and must be a harmless no-op. + if _, err := st.pool.Exec(ctx, `DELETE FROM organizations WHERE id=$1`, org.ID); err != nil { + t.Fatalf("delete org: %v", err) + } + var n int + if err := st.pool.QueryRow(ctx, `SELECT count(*) FROM organizations WHERE id=$1`, org.ID).Scan(&n); err != nil { + t.Fatalf("count org: %v", err) + } + if n != 0 { + t.Fatalf("org still present after delete (n=%d)", n) + } +} diff --git a/internal/store/orgs.go b/internal/store/orgs.go index 3a4014b..007a9e1 100644 --- a/internal/store/orgs.go +++ b/internal/store/orgs.go @@ -235,69 +235,59 @@ func escapeLikePattern(s string) string { // more rows follow. // // Keyset (seek) paging keeps every page cheap regardless of depth — the -// (key, id) comparison rides the sort order — and caps the per-row member and -// repeater counts at the page size. Counts are computed in an inner select so -// the count-based orderings can both sort and seek on them. +// (key, id) comparison rides the sort order. member_count/repeater_count are +// denormalized columns on organizations (trigger-maintained; see migration +// 0033), so every ordering sorts and seeks on an indexed column rather than a +// correlated count subquery computed per org on each page load. func (s *Store) ListPublicOrgsPage(ctx context.Context, p OrgListParams) ([]OrgSummary, bool, error) { var args []any add := func(v any) string { args = append(args, v); return fmt.Sprintf("$%d", len(args)) } - // Search filter applies to the inner select (raw columns). - innerWhere := "" + var where []string + // Substring search over name/description/region (trigram-indexed, migration 0033). if q := strings.TrimSpace(p.Query); q != "" { - like := "%" + escapeLikePattern(q) + "%" - ph := add(like) - innerWhere = fmt.Sprintf( - "WHERE (o.name ILIKE %[1]s OR o.description ILIKE %[1]s OR o.region ILIKE %[1]s)", ph) + ph := add("%" + escapeLikePattern(q) + "%") + where = append(where, fmt.Sprintf( + "(o.name ILIKE %[1]s OR o.description ILIKE %[1]s OR o.region ILIKE %[1]s)", ph)) } - // Ordering and keyset seek apply to the outer select (computed columns - // available). Each seek tuple mirrors its ORDER BY exactly. - var order, keyset string + // Ordering + keyset seek. Each seek tuple mirrors its ORDER BY exactly. + var order string switch p.Sort { case OrgSortName: order = "name ASC, id ASC" if p.HasCursor { - keyset = fmt.Sprintf("(name, id) > (%s, %s)", add(p.AfterName), add(p.AfterID)) + where = append(where, fmt.Sprintf("(name, id) > (%s, %s)", add(p.AfterName), add(p.AfterID))) } case OrgSortRepeaters: order = "repeater_count DESC, id DESC" if p.HasCursor { - keyset = fmt.Sprintf("(repeater_count, id) < (%s, %s)", add(p.AfterCount), add(p.AfterID)) + where = append(where, fmt.Sprintf("(repeater_count, id) < (%s, %s)", add(p.AfterCount), add(p.AfterID))) } case OrgSortNewest: order = "created_at DESC, id DESC" if p.HasCursor { - keyset = fmt.Sprintf("(created_at, id) < (%s, %s)", add(p.AfterTime), add(p.AfterID)) + where = append(where, fmt.Sprintf("(created_at, id) < (%s, %s)", add(p.AfterTime), add(p.AfterID))) } default: // OrgSortMembers order = "member_count DESC, id DESC" if p.HasCursor { - keyset = fmt.Sprintf("(member_count, id) < (%s, %s)", add(p.AfterCount), add(p.AfterID)) + where = append(where, fmt.Sprintf("(member_count, id) < (%s, %s)", add(p.AfterCount), add(p.AfterID))) } } - outerWhere := "" - if keyset != "" { - outerWhere = "WHERE " + keyset + whereClause := "" + if len(where) > 0 { + whereClause = "WHERE " + strings.Join(where, " AND ") } // Fetch one extra row to detect whether a further page exists. limit := add(OrgsPageSize + 1) query := fmt.Sprintf(` - SELECT id, slug, name, description, region, member_count, repeater_count, created_at - FROM ( - SELECT o.id, o.slug, o.name, o.description, o.region, o.created_at, - (SELECT count(*) FROM org_members m WHERE m.org_id = o.id) AS member_count, - (SELECT count(*) FROM repeaters r - JOIN org_members om ON om.org_id = o.id AND om.user_id = r.owner_id - WHERE NOT EXISTS (SELECT 1 FROM org_repeater_excludes e - WHERE e.org_id = o.id AND e.repeater_id = r.id)) AS repeater_count - FROM organizations o - %s - ) t + SELECT o.id, o.slug, o.name, o.description, o.region, o.member_count, o.repeater_count, o.created_at + FROM organizations o %s ORDER BY %s - LIMIT %s`, innerWhere, outerWhere, order, limit) + LIMIT %s`, whereClause, order, limit) rows, err := s.pool.Query(ctx, query, args...) if err != nil {