Optimize orgs list queries

2026-09-01 17:38:15 +00:00 · 2026-07-05 12:46:36 -04:00
parent a232ee6df6
commit 377a9df3b3
3 changed files with 315 additions and 31 deletions
@@ -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;
@@ -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)
+	}
+}
@@ -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 {