Skip to content

Cut remote-DB list latency by batching N+1 lookups (banlist ~5×, comms ~6×) - #1533

Open
maxijabase wants to merge 14 commits into
sbpp:mainfrom
maxijabase:fix/remote-db-query-batching
Open

Cut remote-DB list latency by batching N+1 lookups (banlist ~5×, comms ~6×)#1533
maxijabase wants to merge 14 commits into
sbpp:mainfrom
maxijabase:fix/remote-db-query-batching

Conversation

@maxijabase

Copy link
Copy Markdown
Contributor

Summary

When the panel and MariaDB are geographically far apart, per-row SQL on list pages turns every RTT into wall-clock pain. This PR collapses those N+1 patterns into page-scoped batch queries, adds query-count instrumentation + regression tests, and tightens two batching edge cases (CONSOLE RemovedBy = 0 name resolution, and excluding aid = 0 from GetAllAdmins).

Also included from the same deploy/debug cycle (not N+1, but already on this branch):

  • Operator-facing JWT SB_SECRET_KEY validation (≥32 decoded bytes, shared PHP + entrypoint gate)
  • Effective-key validation against config.php (not a possibly-stale env var)
  • Prod Docker .dockerignore + Apache deny so local config.php / backups / .env cannot bake into or be served from the image

Motivation

A production-shaped setup (app on Railway, MariaDB in Argentina) made public ban/comms lists take tens of seconds per full HTML response. The dominant cost was not “slow SQL” in isolation. It was many sequential round trips (admin names, mods, comments, banlog, demo flags, protest/submission side lookups, etc.) each paying transcontinental RTT.

What changed (DB performance)

Instrumentation

  • Sbpp\Db\Database::resetQueryCount() / getQueryCount() — increments on every query() (prepare), the single choke point for logical statements
  • Fail-closed DB connect: no more leaking raw PDO messages / exiting CLI with status 0 on connect failure

Batched surfaces

Area Change
UserManager::GetAllAdmins() One join query instead of per-aid loads; skips aid <= 0 (CONSOLE) so the cache matches GetUserArray
page.banlist.php Batch admin/mod/RemovedBy name maps, comments, banlog, related lookups
page.commslist.php Same pattern for comm blocks
admin.admins.php Batch ban / nodemo counts for the admins list
admin.bans.php Batch protest + submission queue lookups (current/archive paths)
admin.groups.php Batch group list membership / related lookups

Batching edge cases + tests

  • Collect RemovedBy with !== null (not !empty), so PruneBans / CONSOLE RemovedBy = 0 still enter the name map (BanlistRemovedByConsoleTest)
  • GetAllAdmins excludes aid <= 0 (covered in UserManagerGetAllAdminsQueryCountTest)
  • Query-count integration tests for banlist, commslist, GetAllAdmins, and admin admins / protests / submissions / groups

Measured query budgets stay flat across row counts (e.g. admin admins ≈5, protests ≈4, submissions ≈5, groups ≈4; caps set with small headroom).

Intentional follow-ups (out of this PR)

  • Supporting indexes + updater migrations for the new batch predicates
  • Schema uniqueness / join redesign where duplicate srvgroups.name rows can still amplify work
  • Narrower admin-id projection helpers if call sites need subsets without full GetAllAdmins
  • Archive protest update concurrency (classify from snapshot, update by pid later)
  • Broader instrumentation than the Database::query() wrapper (e.g. raw PDO)

Latency compare (before vs after)

Local harness: interleaved HTTP GETs against old production panel (bans.electricservers.com.ar which is running ghcr.io/sbpp/sourcebans-pp:2.0.2) vs new Railway deploy of this branch, same Argentina DB, full response body drained (not TTFB-only), auth cookies on both sides for admin routes.

SourceBans++ latency compare
  old:     https://bans.electricservers.com.ar
  new:     https://sourcebans-pp-production.up.railway.app
  samples: 3 timed + 1 warmup (interleaved)
  timeout: 120000ms
  cookie:  old=set  new=set
  routes:  dashboard, banlist, banlist p2, commslist, servers, admin admins, admin protests, admin submissions, admin groups

→ dashboard (/index.php?p=home)
  warm  old=   9.01s  new=   6.33s
  s01  old=   8.66s  new=   6.06s
  s02  old=   8.51s  new=   6.19s
  s03  old=   8.37s  new=   6.22s

→ banlist (/index.php?p=banlist)
  warm  old=  45.29s  new=   9.08s
  s01  old=  47.64s  new=   8.61s
  s02  old=  46.90s  new=   8.72s
e=2)
  warm  old=  44.95s  new=   8.56s
  s01  old=  46.86s  new=   8.53s
  s02  old=  46.30s  new=   8.59s
  s03  old=  46.03s  new=   8.73s

→ commslist (/index.php?p=commslist)
  warm  old=  42.46s  new=   6.59s
  s01  old=  43.22s  new=   6.71s
  s02  old=  43.42s  new=   6.71s
  s03  old=  42.41s  new=   6.67s

→ servers (/index.php?p=servers)
  warm  old=   5.74s  new=   3.29s
  s01  old=   5.83s  new=   3.12s
  s02  old=   5.42s  new=   3.13s
  s03  old=   5.72s  new=   3.17s

→ admin admins (/index.php?p=admin&c=admins&section=admins)
  warm  old=  28.77s  new=   6.43s
  s01  old=  29.64s  new=   6.67s
  s02  old=  29.13s  new=   6.59s
  s03  old=  28.96s  new=   6.53s

→ admin protests (/index.php?p=admin&c=bans&section=protests)
  warm  old=   9.09s  new=   4.37s
  s01  old=   9.16s  new=   4.42s
  s02  old=   9.31s  new=   4.49s
  s03  old=   8.98s  new=   4.39s

→ admin submissions (/index.php?p=admin&c=bans&section=submissions)
  warm  old=  32.64s  new=   4.84s
  s01  old=  34.39s  new=   4.79s
  s02  old=  32.83s  new=   4.81s
  s03  old=  32.77s  new=   4.76s

→ admin groups (/index.php?p=admin&c=groups&section=list)
  warm  old=  10.88s  new=   5.10s
  s01  old=  10.69s  new=   5.23s
  s02  old=  10.17s  new=   5.24s
  s03  old=  10.28s  new=   5.22s

Summary (timed samples only; median is the headline number)

route                old med   new med   old p95   new p95  delta (median)                ok old/new
----------------------------------------------------------------------------------------------------
dashboard              8.51s     6.19s     8.66s     6.22s  1.4x faster (−27%)                   3/3
banlist               47.59s     8.61s    47.64s     8.72s  5.5x faster (−82%)                   3/3
banlist p2            46.30s     8.59s    46.86s     8.73s  5.4x faster (−81%)                   3/3
commslist             43.22s     6.71s    43.42s     6.71s  6.4x faster (−84%)                   3/3
servers                5.72s     3.13s     5.83s     3.17s  1.8x faster (−45%)                   3/3
admin admins          29.13s     6.59s    29.64s     6.67s  4.4x faster (−77%)                   3/3
admin protests         9.16s     4.42s     9.31s     4.49s  2.1x faster (−52%)                   3/3
admin submissions     32.83s     4.79s    34.39s     4.81s  6.9x faster (−85%)                   3/3
admin groups          10.28s     5.23s    10.69s     5.24s  2.0x faster (−49%)                   3/3

Notes
  • Interleaved old→new hits share the Argentina DB fairly.
  • Full response body is drained (not TTFB-only).
  • Public routes only unless --cookie / --include-auth.
  • Absolute times still include Railway↔Argentina RTT; compare ratios.

Notes for reviewers:

  • Absolute times still include Railway (California/US West) <-> Argentina (Buenos Aires) RTT. Compare ratios, not absolute seconds alone.
  • Interleaving shares DB load fairly between old and new.
  • Servers/dashboard improve less when they were never the N+1 hot path; banlist/comms/admins/submissions are the load-bearing surfaces.

Also on this branch (not N+1)

  1. .dockerignore - exclude web/config.php, web/config.php.* (keep config.php.template), host .env files, vendor, caches, demos from docker/Dockerfile.prod context. Without the config.php exclusion, a local ./sbpp.sh config (DB_HOST=db) can ship inside the image; the entrypoint skips render_config and containers fail with getaddrinfo for db failed even with correct env vars.

  2. Apache - deny config.php and backup suffixes (config.php.bak, …) via <FilesMatch> in sbpp-prod.conf so credential backups are not served as plaintext.

  3. JWT + prod-entrypoint.sh + Docker quickstart - SB_SECRET_KEY must be base64 that decodes to at least 32 bytes (JWT::signingKeyFromSecret, MIN_SECRET_BYTES = 32). Invalid base64 and short-but-valid base64 both fail closed with an operator-readable message (openssl rand -base64 47). The entrypoint validates the effective key in config.php after render (stale env cannot block a healthy install; fixed env cannot mask a bad persisted key). Fresh config writes still pre-check the env value before persisting it.

  4. Tests - JwtSecretKeyTest, DockerIgnoreSecretsTest, expanded ProdApacheConfigTest; compose mounts .dockerignore for local PHPUnit symmetry with CI.

Test plan

  • ./sbpp.sh test --filter=QueryCount
  • ./sbpp.sh test --filter=UserManagerGetAllAdmins
  • ./sbpp.sh test --filter=BanlistRemovedByConsole
  • ./sbpp.sh test --filter=JwtSecretKey (invalid + short base64 rejected; 32-byte floor accepted)
  • ./sbpp.sh test --filter='ProdApacheConfig|DockerIgnoreSecrets'
  • Spot-check banlist / commslist / admin admins / protests / submissions / groups with seeded data
  • Confirm CONSOLE-removed rows still show the CONSOLE name
  • Optional: rebuild prod image from a worktree that has a local web/config.php and confirm the baked image does not contain that file
  • Optional: confirm entrypoint dies on bad env before writing a fresh config.php, and validates the key inside an existing config.php even when env differs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant