Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions db/migrations/20260517000000_events_has_embedding.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
-- migrate:up

-- has_embedding mirrors "does event_embeddings have a row for this event_id".
-- The embed-backfill scheduler today does a 1.27s seq scan + hash anti-join
-- on the 1.15M-row events table every 5 min (pg_stat_statements rank #1 by
-- total_exec_time, see lobu#767 postmortem). With this column + a partial
-- index (added in a follow-up migration after backfill), the scheduler
-- becomes a tiny index lookup over the actually-missing-embedding rows.
--
-- This migration only adds the column and the maintenance triggers. The
-- column is intentionally:
-- * nullable: ADD COLUMN <bool> NULL is O(1) metadata-only in PG 11+; a
-- DEFAULT would rewrite all 1.15M rows under ACCESS EXCLUSIVE (the same
-- trap that timed out 20260516200000_events_search_tsv).
-- * not backfilled here: existing rows are populated by a batched script
-- (scripts/backfill-events-has-embedding.sql) that runs outside the

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix stale script path in migration comment (Line 16).

The comment points to scripts/backfill-events-has-embedding.sql, but this PR adds scripts/backfill-events-has-embedding.sh.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@db/migrations/20260517000000_events_has_embedding.sql` at line 16, Update the
stale comment in the migration file that references
"scripts/backfill-events-has-embedding.sql" so it reflects the actual script
added by this PR ("scripts/backfill-events-has-embedding.sh"); locate the
comment text in db/migrations/20260517000000_events_has_embedding.sql (the line
containing scripts/backfill-events-has-embedding.sql) and change the file
extension in the comment to .sh to match the new script name.

-- Helm hook so it can pace itself with statement_timeout headroom.
--
-- Until backfill finishes, has_embedding IS NULL means "unknown" for old
-- rows; new rows get has_embedding flipped by the triggers below. The
-- partial index (next migration) treats NULL the same as FALSE so the
-- scheduler keeps working.

ALTER TABLE public.events ADD COLUMN has_embedding boolean;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Set new events to missing embeddings

Because the new column is nullable with no default and only event_embeddings has maintenance triggers, any event inserted after the one-time backfill finishes but before its embedding row exists will keep has_embedding = NULL. The planned partial-index path uses WHERE NOT has_embedding (and the current scheduler's missing-embedding predicate is in packages/server/src/scheduled/trigger-embed-backfill.ts), so those new NULL rows would be excluded rather than queued for embedding. Add a default/insert-side maintenance path so freshly created events start as false until the embedding insert flips them to true.

Useful? React with 👍 / 👎.


CREATE OR REPLACE FUNCTION public.event_embeddings_after_insert() RETURNS trigger AS $$
BEGIN
UPDATE public.events SET has_embedding = true WHERE id = NEW.event_id;
RETURN NEW;
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION public.event_embeddings_after_delete() RETURNS trigger AS $$
BEGIN
UPDATE public.events SET has_embedding = false WHERE id = OLD.event_id;
RETURN OLD;
END;
$$ LANGUAGE plpgsql;

DROP TRIGGER IF EXISTS trg_event_embeddings_after_insert ON public.event_embeddings;
CREATE TRIGGER trg_event_embeddings_after_insert
AFTER INSERT ON public.event_embeddings
FOR EACH ROW EXECUTE FUNCTION public.event_embeddings_after_insert();

DROP TRIGGER IF EXISTS trg_event_embeddings_after_delete ON public.event_embeddings;
CREATE TRIGGER trg_event_embeddings_after_delete
AFTER DELETE ON public.event_embeddings
FOR EACH ROW EXECUTE FUNCTION public.event_embeddings_after_delete();

-- migrate:down

DROP TRIGGER IF EXISTS trg_event_embeddings_after_insert ON public.event_embeddings;
DROP TRIGGER IF EXISTS trg_event_embeddings_after_delete ON public.event_embeddings;
DROP FUNCTION IF EXISTS public.event_embeddings_after_insert();
DROP FUNCTION IF EXISTS public.event_embeddings_after_delete();
ALTER TABLE public.events DROP COLUMN IF EXISTS has_embedding;
80 changes: 80 additions & 0 deletions scripts/backfill-events-has-embedding.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
#!/bin/bash
# Batched backfill for events.has_embedding (added in migration
# 20260517000000_events_has_embedding.sql).
#
# Why batched and not in the migration: a single UPDATE over 1.15M rows
# would hold an exclusive row-level lock for ~minute on a hot table while
# WAL-amplifying every row. Doing it in batches of 10k means each transaction
# is bounded (~1s) so VACUUM can keep up, autovacuum doesn't fall behind, and
# the API stays responsive.
#
# Idempotent: only touches rows where has_embedding IS NULL, so re-running
# after a partial run picks up where it left off.
#
# Run from the repo root:
# DATABASE_URL="postgres://..." ./scripts/backfill-events-has-embedding.sh
#
# Operational note: takes ~5-10 min for 1.15M rows on a small Postgres. Safe
# to ctrl-C and resume; safe to run alongside live traffic.

set -euo pipefail

if [ -z "${DATABASE_URL:-}" ]; then
echo "DATABASE_URL not set" >&2
exit 1
fi

BATCH_SIZE="${BATCH_SIZE:-10000}"
SLEEP_BETWEEN_BATCHES="${SLEEP_BETWEEN_BATCHES:-0.1}"

Comment on lines +27 to +29

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate batch/sleep env vars before using them in SQL and sleep.

BATCH_SIZE is interpolated into LIMIT (Lines 53-54). A bad value causes runtime SQL errors (or unsafe huge batches). Add numeric/positive guards up front.

🛠️ Suggested guardrails
 BATCH_SIZE="${BATCH_SIZE:-10000}"
 SLEEP_BETWEEN_BATCHES="${SLEEP_BETWEEN_BATCHES:-0.1}"
+
+if ! [[ "$BATCH_SIZE" =~ ^[0-9]+$ ]] || [ "$BATCH_SIZE" -le 0 ]; then
+  echo "BATCH_SIZE must be a positive integer" >&2
+  exit 1
+fi
+
+if ! [[ "$SLEEP_BETWEEN_BATCHES" =~ ^[0-9]+([.][0-9]+)?$ ]]; then
+  echo "SLEEP_BETWEEN_BATCHES must be a non-negative number" >&2
+  exit 1
+fi

Also applies to: 49-55

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/backfill-events-has-embedding.sh` around lines 27 - 29, Validate and
normalize the BATCH_SIZE and SLEEP_BETWEEN_BATCHES env vars before they are
interpolated into SQL or passed to sleep: ensure BATCH_SIZE is a positive
integer (>=1) and SLEEP_BETWEEN_BATCHES is a non-negative number (float
allowed), otherwise set them to safe defaults or exit with an error; perform
this check immediately after their assignment (the BATCH_SIZE and
SLEEP_BETWEEN_BATCHES variables at the top of the script) so the sanitized
values are used in the SQL LIMIT and the sleep call.

echo "[backfill] BATCH_SIZE=$BATCH_SIZE SLEEP_BETWEEN_BATCHES=${SLEEP_BETWEEN_BATCHES}s"

remaining_count() {
psql "$DATABASE_URL" -tAc "SELECT count(*) FROM public.events WHERE has_embedding IS NULL"
}

total_remaining=$(remaining_count)
echo "[backfill] $total_remaining rows have has_embedding IS NULL"

if [ "$total_remaining" = "0" ]; then
echo "[backfill] nothing to do"
exit 0
fi

batches=0
while true; do
# Each batch is one transaction. Update up to $BATCH_SIZE rows where the
# column is still unknown. UPDATE FROM joins to event_embeddings to set
# the right value; rows with no matching embedding row get FALSE.
rows_updated=$(psql "$DATABASE_URL" -tAc "
WITH batch AS (
SELECT id FROM public.events
WHERE has_embedding IS NULL
LIMIT $BATCH_SIZE
FOR UPDATE SKIP LOCKED
)
UPDATE public.events e
SET has_embedding = (emb.event_id IS NOT NULL)
FROM batch b
LEFT JOIN public.event_embeddings emb ON emb.event_id = b.id
WHERE e.id = b.id
RETURNING 1
" | wc -l | tr -d ' ')

batches=$((batches + 1))

if [ "$rows_updated" = "0" ]; then
echo "[backfill] no rows updated in batch $batches — done"
break
fi

if [ $((batches % 10)) -eq 0 ]; then
current_remaining=$(remaining_count)
echo "[backfill] batch $batches: ~${rows_updated} rows updated this batch, ${current_remaining} remaining"
fi

sleep "$SLEEP_BETWEEN_BATCHES"
done

final_remaining=$(remaining_count)
echo "[backfill] complete: $final_remaining rows still have has_embedding IS NULL (expect 0)"
Loading