perf(feed): stop every feed page from aggregating the whole event

v_feed computed like_count/comment_count with LEFT JOINs and a GROUP BY. Postgres
CAN push `event_id = $1` and the keyset predicate through the view — verified with
EXPLAIN, it uses idx_upload_event_created_id — but it CANNOT push ORDER BY ...
LIMIT across a GroupAggregate. So every request aggregated every upload in the
event, times its likes and comments, and only then sorted and took 21 rows. Cost
grew with the event, not with the page.

Measured on a throwaway database seeded to a real reception (1000 uploads, 100
guests, 27k likes, 10k comments), same query, same data:

  before   GroupAggregate (actual rows=1001) -> Sort -> Limit    449 ms
  after    Index Scan (actual rows=21) -> Limit -> SubPlans      0.58 ms

migration 022 replaces the joins with correlated scalar subqueries, which puts the
counts ABOVE the Limit so they run 21 times instead of 1001. Exactly equivalent,
not merely close: "like" is keyed (upload_id, user_id) so COUNT(DISTINCT user_id)
== count(*), comment.id is the PK so COUNT(DISTINCT c.id) == count(*), and the
GROUP BY was on u.id so it was already one row per upload. Column names, order and
types are unchanged, so no Rust changes. No new index needed — idx_like_upload and
idx_comment_upload already match the subqueries.

Note the existing load harness cannot see any of this: e2e/loadtest/driver.mjs
creates no likes and no comments, so the expensive path had never been exercised.

The amplifier, feed/+page.svelte: every open feed subscribed to `upload-processed`
and refetched page 1 — the most expensive page — so ~100 open feeds each fired one
per completed upload. Now gated on whether this client actually shows the card
that changed, and the debounce is jittered, because a fixed delay just moves a
simultaneous herd 800 ms later. Nothing is lost by skipping: a client without the
card also missed its `new-upload`, and the reconnect `feed-delta` already
schedules a refresh.

Load shedding, because the above reduces the risk rather than removing it: db.rs
set no acquire_timeout, so sqlx's 30 s default applied — longer than the
frontend's own 20 s fetch timeout, meaning the browser gave up while the server
kept holding the slot and the work was done for nobody. Now 5 s, and PoolTimedOut
maps to 503 + Retry-After instead of a generic 500. That mattered because the
upload queue classifies 5xx as transient and retries: a 500 sent the retries
straight back into the saturated pool with nothing to pace them. PoolClosed stays
Internal — it only occurs during shutdown, where a 503 would invite a retry
against a server that is going away.

The Retry-After extraction in into_response matches on variants, so unlike
message() a missing arm is not a compile error — it would silently drop the
header. Pinned by a test covering both retry-carrying variants.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-07-31 23:01:42 +02:00
parent 3f0f9c098b
commit f275de5c8f
5 changed files with 217 additions and 10 deletions

View File

@@ -0,0 +1,26 @@
-- Restore the 016 definition verbatim.
DROP VIEW IF EXISTS v_feed;
CREATE VIEW v_feed AS
SELECT
u.id,
u.event_id,
u.user_id,
usr.display_name AS uploader_name,
usr.is_banned,
usr.uploads_hidden,
u.preview_path,
u.thumbnail_path,
u.display_path,
u.mime_type,
u.caption,
u.created_at,
COUNT(DISTINCT l.user_id) AS like_count,
COUNT(DISTINCT c.id) AS comment_count
FROM upload u
JOIN "user" usr ON u.user_id = usr.id
LEFT JOIN "like" l ON l.upload_id = u.id
LEFT JOIN comment c ON c.upload_id = u.id AND c.deleted_at IS NULL
WHERE u.deleted_at IS NULL
AND usr.uploads_hidden = FALSE
AND usr.is_banned = FALSE
GROUP BY u.id, usr.display_name, usr.is_banned, usr.uploads_hidden;

View File

@@ -0,0 +1,55 @@
-- Make a feed page cost a page, not the whole event.
--
-- The previous definition (016) computed like_count/comment_count with LEFT JOINs and a
-- GROUP BY. Postgres CAN push the `event_id = $1` qual and the keyset predicate through the
-- view — verified with EXPLAIN, it uses idx_upload_event_created_id — but it CANNOT push
-- ORDER BY ... LIMIT across a GroupAggregate. So every feed request aggregated every upload in
-- the event (times its likes and comments) and only then sorted and took 21 rows. The cost
-- grew with the event, not with the page, and page 1 — the most expensive one — is exactly
-- what refreshFeedInPlace refetches on every completed upload, from every open feed in the
-- venue.
--
-- Correlated scalar subqueries move the counts ABOVE the Limit in the plan: they are evaluated
-- once per returned row, so 21 index lookups instead of a full aggregation.
--
-- The rewrite is EXACTLY equivalent, not merely close:
-- * "like" is keyed (upload_id, user_id), so COUNT(DISTINCT l.user_id) == count(*).
-- * comment.id is the primary key, so COUNT(DISTINCT c.id) == count(*).
-- * one row per upload either way — the GROUP BY was on u.id.
-- Column names, order and types are unchanged (count(*) and COUNT(DISTINCT ...) are both
-- bigint), so no Rust code changes.
--
-- No new index needed: idx_like_upload plus the (upload_id, user_id) PK serve the like
-- subquery, and idx_comment_upload ... WHERE deleted_at IS NULL matches the comment
-- subquery's predicate exactly.
--
-- One thing a future editor needs to know: the hashtag-filtered feed joins upload_hashtag
-- against this view. That was safe before only because the GROUP BY collapsed the join
-- fan-out; it is safe now because the view is one row per upload and upload_hashtag is keyed
-- (upload_id, hashtag_id) with a single tag filtered. Adding a second tag filter would need
-- fresh thought.
-- Not CASCADE: if something ever comes to depend on this view, the migration should fail
-- loudly rather than silently drop it.
DROP VIEW IF EXISTS v_feed;
CREATE VIEW v_feed AS
SELECT
u.id,
u.event_id,
u.user_id,
usr.display_name AS uploader_name,
usr.is_banned,
usr.uploads_hidden,
u.preview_path,
u.thumbnail_path,
u.display_path,
u.mime_type,
u.caption,
u.created_at,
(SELECT count(*) FROM "like" l WHERE l.upload_id = u.id) AS like_count,
(SELECT count(*) FROM comment c WHERE c.upload_id = u.id AND c.deleted_at IS NULL) AS comment_count
FROM upload u
JOIN "user" usr ON u.user_id = usr.id
WHERE u.deleted_at IS NULL
AND usr.uploads_hidden = FALSE
AND usr.is_banned = FALSE;

View File

@@ -4,6 +4,15 @@ use sqlx::postgres::PgPoolOptions;
const DEFAULT_MAX_CONNECTIONS: u32 = 10; const DEFAULT_MAX_CONNECTIONS: u32 = 10;
/// How long a request waits for a pool connection before being shed.
///
/// sqlx's default is 30 s — longer than the frontend's own 20 s fetch timeout (api.ts
/// TIMEOUT_MS), so under saturation the browser gave up while the server kept holding the
/// slot: the client saw a timeout, the server saw a completed request, and the work was done
/// for nobody. Failing fast sheds load instead of compounding it, and `From<sqlx::Error>` in
/// error.rs turns the timeout into a 503 with a Retry-After rather than a bare 500.
const ACQUIRE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5);
/// SQLSTATE for `invalid_password`. /// SQLSTATE for `invalid_password`.
const PG_INVALID_PASSWORD: &str = "28P01"; const PG_INVALID_PASSWORD: &str = "28P01";
@@ -54,6 +63,7 @@ pub async fn create_pool(database_url: &str) -> Result<PgPool> {
let pool = match PgPoolOptions::new() let pool = match PgPoolOptions::new()
.max_connections(max_connections) .max_connections(max_connections)
.acquire_timeout(ACQUIRE_TIMEOUT)
.connect(database_url) .connect(database_url)
.await .await
{ {

View File

@@ -19,6 +19,12 @@ pub enum AppError {
/// the client can treat it as *terminal* (413, no retry) instead of backing off and /// the client can treat it as *terminal* (413, no retry) instead of backing off and
/// retrying a permanently-failing upload forever. /// retrying a permanently-failing upload forever.
QuotaExceeded(String), QuotaExceeded(String),
/// The server is temporarily unable to serve this request — currently only pool
/// saturation. Distinct from `Internal` because it is TRANSIENT and the client should be
/// told so: a 500 reads as "this request is broken", while a 503 + Retry-After reads as
/// "come back shortly", which is what the upload queue's retry classifier needs to make
/// the right call. Second field: optional retry-after seconds.
ServiceUnavailable(String, Option<u64>),
Internal(anyhow::Error), Internal(anyhow::Error),
} }
@@ -33,6 +39,9 @@ impl AppError {
Self::Conflict(_) => (StatusCode::CONFLICT, "conflict"), Self::Conflict(_) => (StatusCode::CONFLICT, "conflict"),
Self::TooManyRequests(..) => (StatusCode::TOO_MANY_REQUESTS, "too_many_requests"), Self::TooManyRequests(..) => (StatusCode::TOO_MANY_REQUESTS, "too_many_requests"),
Self::QuotaExceeded(_) => (StatusCode::PAYLOAD_TOO_LARGE, "quota_exceeded"), Self::QuotaExceeded(_) => (StatusCode::PAYLOAD_TOO_LARGE, "quota_exceeded"),
Self::ServiceUnavailable(..) => {
(StatusCode::SERVICE_UNAVAILABLE, "service_unavailable")
}
Self::Internal(_) => (StatusCode::INTERNAL_SERVER_ERROR, "internal_error"), Self::Internal(_) => (StatusCode::INTERNAL_SERVER_ERROR, "internal_error"),
} }
} }
@@ -46,6 +55,7 @@ impl AppError {
| Self::NotFound(msg) | Self::NotFound(msg)
| Self::Conflict(msg) => msg.clone(), | Self::Conflict(msg) => msg.clone(),
Self::TooManyRequests(msg, _) => msg.clone(), Self::TooManyRequests(msg, _) => msg.clone(),
Self::ServiceUnavailable(msg, _) => msg.clone(),
Self::QuotaExceeded(msg) => msg.clone(), Self::QuotaExceeded(msg) => msg.clone(),
Self::Internal(err) => { Self::Internal(err) => {
tracing::error!("internal error: {err:#}"); tracing::error!("internal error: {err:#}");
@@ -58,10 +68,13 @@ impl AppError {
impl IntoResponse for AppError { impl IntoResponse for AppError {
fn into_response(self) -> Response { fn into_response(self) -> Response {
let (status, code) = self.status_and_code(); let (status, code) = self.status_and_code();
let retry_after_secs = if let Self::TooManyRequests(_, Some(secs)) = &self { // BOTH retry-carrying variants must be matched here. `message()` would fail to
Some(*secs) // compile on a missing arm; this one would not — it would silently drop the header and
} else { // the `retry_after_secs` body field, which is exactly the sort of omission that only
None // shows up under the load the 503 exists for.
let retry_after_secs = match &self {
Self::TooManyRequests(_, secs) | Self::ServiceUnavailable(_, secs) => *secs,
_ => None,
}; };
let message = self.message(); let message = self.message();
@@ -93,6 +106,84 @@ impl From<anyhow::Error> for AppError {
impl From<sqlx::Error> for AppError { impl From<sqlx::Error> for AppError {
fn from(err: sqlx::Error) -> Self { fn from(err: sqlx::Error) -> Self {
Self::Internal(err.into()) match err {
// Pool saturation is load, not a bug. Reporting it as a 500 was actively harmful:
// the frontend's upload-queue classifier treats 5xx as transient and retries, so
// the retries piled straight back into the saturated pool with no Retry-After to
// pace them. A 503 says the same thing honestly and carries the backoff.
//
// `PoolClosed` stays `Internal` — it only happens during shutdown, where a 503
// would invite a retry against a server that is going away.
sqlx::Error::PoolTimedOut => {
tracing::warn!("database pool exhausted; shedding a request with 503");
Self::ServiceUnavailable(
"Server ist gerade ausgelastet. Bitte versuche es in ein paar Sekunden erneut."
.into(),
Some(POOL_TIMEOUT_RETRY_AFTER_SECS),
)
}
other => Self::Internal(other.into()),
}
}
}
/// Retry-After for a shed request. Short: pool saturation clears in seconds once the queue
/// drains, and a long value would make a brief spike feel like an outage.
const POOL_TIMEOUT_RETRY_AFTER_SECS: u64 = 3;
#[cfg(test)]
mod tests {
use super::*;
/// `into_response` extracts `retry_after_secs` by MATCHING ON VARIANTS, so unlike
/// `message()` a missing arm is not a compile error — it silently drops the header. Pin the
/// behaviour for both retry-carrying variants.
#[test]
fn both_retry_carrying_variants_emit_retry_after() {
for err in [
AppError::TooManyRequests("slow down".into(), Some(42)),
AppError::ServiceUnavailable("busy".into(), Some(3)),
] {
let expected = match &err {
AppError::TooManyRequests(_, Some(s)) | AppError::ServiceUnavailable(_, Some(s)) => {
s.to_string()
}
_ => unreachable!(),
};
let resp = err.into_response();
assert_eq!(
resp.headers()
.get(axum::http::header::RETRY_AFTER)
.and_then(|v| v.to_str().ok()),
Some(expected.as_str()),
"a shed/throttled client must be told when to come back"
);
}
}
/// Pool saturation is load, not a bug. A 500 makes the frontend's retry classifier pile
/// straight back into the saturated pool with no backoff to pace it.
#[test]
fn pool_exhaustion_sheds_with_503_but_shutdown_does_not() {
let shed: AppError = sqlx::Error::PoolTimedOut.into();
assert_eq!(
shed.status_and_code(),
(StatusCode::SERVICE_UNAVAILABLE, "service_unavailable")
);
// PoolClosed only happens during shutdown; a 503 there would invite a retry against a
// server that is going away.
let closing: AppError = sqlx::Error::PoolClosed.into();
assert_eq!(
closing.status_and_code(),
(StatusCode::INTERNAL_SERVER_ERROR, "internal_error")
);
// Everything else must keep its existing mapping.
let missing: AppError = sqlx::Error::RowNotFound.into();
assert_eq!(
missing.status_and_code(),
(StatusCode::INTERNAL_SERVER_ERROR, "internal_error")
);
} }
} }

View File

@@ -273,7 +273,23 @@
// A processed upload gains preview/thumbnail URLs. Coalesce bursts (bulk // A processed upload gains preview/thumbnail URLs. Coalesce bursts (bulk
// uploads fire one per file) into a single in-place merge so the feed // uploads fire one per file) into a single in-place merge so the feed
// neither hammers the server nor collapses to page 1 / loses scroll. // neither hammers the server nor collapses to page 1 / loses scroll.
onSseEvent('upload-processed', () => scheduleInPlaceRefresh()), onSseEvent('upload-processed', (data) => {
// Only refetch if this client actually SHOWS the card that changed. Without the
// membership test every open feed in the venue (~100 at a reception) fired a
// page-1 refetch for every completed upload — a self-inflicted thundering herd
// on the most expensive page, triggered by the most common event.
//
// Nothing is lost by skipping: a client that doesn't have the card also missed
// its `new-upload`, and the reconnect `feed-delta` below already calls
// scheduleInPlaceRefresh().
try {
const { upload_id } = JSON.parse(data) as { upload_id: string };
if (!uploads.some((u) => u.id === upload_id)) return;
} catch {
return;
}
scheduleInPlaceRefresh();
}),
onSseEvent('upload-deleted', (data) => { onSseEvent('upload-deleted', (data) => {
try { try {
const payload = JSON.parse(data) as { upload_id: string }; const payload = JSON.parse(data) as { upload_id: string };
@@ -399,12 +415,21 @@
// Debounced page-1 fetch that *merges* (updates existing cards in place, prepends // Debounced page-1 fetch that *merges* (updates existing cards in place, prepends
// genuinely new ones) rather than replacing the array — preserves scroll and any // genuinely new ones) rather than replacing the array — preserves scroll and any
// pages already loaded below the fold. // pages already loaded below the fold.
/** Debounce floor. */
const IN_PLACE_REFRESH_MIN_MS = 800;
/** Jitter added on top. Every open feed receives the same SSE at the same instant, so a
* fixed delay just moves the herd 800 ms later instead of spreading it. */
const IN_PLACE_REFRESH_JITTER_MS = 1200;
function scheduleInPlaceRefresh() { function scheduleInPlaceRefresh() {
if (inPlaceRefreshTimer) return; if (inPlaceRefreshTimer) return;
inPlaceRefreshTimer = setTimeout(() => { inPlaceRefreshTimer = setTimeout(
inPlaceRefreshTimer = null; () => {
void refreshFeedInPlace(); inPlaceRefreshTimer = null;
}, 800); void refreshFeedInPlace();
},
IN_PLACE_REFRESH_MIN_MS + Math.random() * IN_PLACE_REFRESH_JITTER_MS
);
} }
async function refreshFeedInPlace() { async function refreshFeedInPlace() {