Skip to content

perf(product-analytics): bound the insight upgrade scan by id window - #111230

Draft
posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/perfproduct-analytics-bound-the-insight-f404ad
Draft

posthog[bot] wants to merge 1 commit into
masterfrom
posthog-self-driving/perfproduct-analytics-bound-the-insight-f404ad

Conversation

@posthog

@posthog posthog Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • The upgrade-queries Temporal workflow loads the main Postgres primary every 6 hours. One call to get_insights_to_migrate can run for minutes, close to the 5-minute activity timeout. When a call times out, the insight schema upgrade backlog stops.
  • The WHERE clause is an OR of recursive jsonpath checks. No index can serve it, so Postgres parses the query JSONB of each row it reads.
  • With LIMIT 100 and no upper bound, a page that has few matches reads most of posthog_dashboarditem in one statement.

Origin

  • pganalyze
  • First signal: 2026-10-03
  • Inbox report: open
  • Likely cause: 4829393
  • Task started by: auto-start, after the report was rated P3 and ready to fix

Changes

  • Each statement now scans a fixed id window (id > after AND id <= after + 5000). One call scans at most 20 windows, or stops when it has a full batch, then returns. The cursor moves past empty windows.
  • The activity returns done when the cursor reaches max(id). Before, the workflow stopped when it got an empty page, but an empty window is now a valid page.
  • The workflow skips migrate_insights_batch for an empty page and stops on done. A workflow.patched("upgrade-queries-id-window") gate keeps the previous command sequence for histories from before this change.
  • The query no longer uses DISTINCT on the primary key, and it sends the bounds as parameters, not as f-string values.
  • No user-visible change: this is a background job only.
flowchart LR
  subgraph Before
    A1[get page: id > after LIMIT 100] -->|empty| D1[finish]
    A1 -->|ids| M1[migrate] --> A1
  end
  subgraph After
    A2[get page: up to 20 windows of 5k ids] -->|ids| M2[migrate] --> C2{done?}
    A2 -->|empty| C2
    C2 -->|no| A2
    C2 -->|yes| D2[finish]
  end
  style A2 fill:#1D4AFF,color:#fff
  style C2 fill:#F9BD2B
Loading

Note

The 5k window size is an estimate. One window should take well under a second, but nobody has measured it in production. After the next scheduled run, compare the max and mean time per call in pganalyze.

How did you test this code?

  • Ran pytest products/product_analytics/backend/temporal/tests/test_upgrade_queries_workflow.py against a local Postgres. All 5 tests pass, including the end-to-end workflow test. ruff check and ruff format are clean.
  • Did not run the query on a production-sized table. The performance gain is not measured.

Test rationale: The new test test_get_insights_to_migrate_activity_pages_through_small_id_windows pages with batch_size=2 and scan_window_size=2. It fails if the scan stops at the first empty window, or if a page that ends partway through a window drops the remaining matches. The existing single-call test does not cover these cases because it gets all rows in one page. The existing snapshot test now replaces the id bounds with a fixed value, so the snapshot does not change when the test database's id sequence changes.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

None.

🤖 Agent context

Autonomy: Fully autonomous

Agent: Claude Code, claude-opus-5-5

  • Skills invoked: /writing-tests. Followed the repo Temporal workflow-versioning rule (workflow.patched gate).
  • Duplicate search: no open PR touches this workflow.
  • Not done: skipping ids that failed in earlier runs. In one run, the forward-only cursor already skips failed ids. Skipping them across runs needs a persistent store, which is outside the scope of this PR.
  • Public artifact: the committed code and fixtures contain no session material.

Created with PostHog Desktop from this inbox report.

🤖 Generated with Claude Code

get_insights_to_migrate parsed the query JSONB of every row until it found a full page, so a page with few matches read most of posthog_dashboarditem in one statement. Each statement now scans a fixed id window, and the cursor advances past empty windows. The workflow ends on a done flag, gated by workflow.patched for in-flight histories.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 8d89fd12-0501-4ef9-8dff-5543466a1a0e
@posthog posthog Bot added the self-driving label Oct 3, 2026
@trunk-io

trunk-io Bot commented Oct 3, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Comment density — 6% of added code lines are comments (4 of 71)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/product_analytics/backend/temporal/upgrade_queries_activities.py 3 29
products/product_analytics/backend/temporal/upgrade_queries_workflow.py 1 17

This check does not block merging. It updates on every push and clears when the share drops.

@hosthog

hosthog Bot commented Oct 3, 2026

Copy link
Copy Markdown

HostHog preview — posthog-desktop-web

Latest build (440e93a): https://c2237139aa764b9b91a8643bb0975b19.hosthog.dev

Employee-gated; every push gets a fresh URL whose content never changes. All previews stop serving when the PR closes.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (8)
.agents/security.md — configured
.agents/skills/sending-notifications/SKILL.md — configured
docs/published/handbook/engineering/type-system.md — configured
.agents/skills/writing-tests/SKILL.md — configured
docs/internal/person-data-access.md — configured
.agents/skills/adopting-generated-api-types/SKILL.md — configured
.claude/commands/conventions.md — configured
.agents/skills/writing-code-comments/SKILL.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: 45f752c7-6832-4a98-9f19-e50a56d6f1b5
📥 Commits

Reviewing files that changed from the base of the PR and between e7ba672 and 440e93a.

📒 Files selected for processing (4)
  • products/product_analytics/backend/temporal/tests/__snapshots__/test_upgrade_queries_workflow.ambr
  • products/product_analytics/backend/temporal/tests/test_upgrade_queries_workflow.py
  • products/product_analytics/backend/temporal/upgrade_queries_activities.py
  • products/product_analytics/backend/temporal/upgrade_queries_workflow.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The activity now scans dashboard-item IDs in bounded windows and returns a completion flag with each result. The workflow uses that flag to determine when to exit, while continuing to migrate nonempty pages. Tests cover the captured SQL and pagination across multiple activity calls.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 440e9

No concrete migration-pagination failure remains identified. The change is mergeable after normal checks; the scheduled run can provide the planned timing measurement.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 440e9

The bounded scan reduces database work without visibly adding access or privileges. However, old and new job versions disagree on completion. Unless rollout keeps compatible versions together, upgrades can stop early or repeatedly poll the shared database.

Retained concerns

  • Medium · reliability · inferred: The changed completion semantics are exposed through the existing activity name. During mixed-version execution or rollback, a legacy workflow can interpret an empty nonterminal window as completion and strand later upgrades. A patched workflow receiving legacy terminal results can also repeat database discovery if missing done values become False. This weakens migration recovery and failure containment for a job sharing database and worker resources. Compatible deployment sequencing or version affinity was not established.
Security review details

Security Blast Radius

  • observed — The migration remains table-wide rather than tenant-scoped: discovery reads dashboard-item IDs without a tenant predicate, and migration selects those IDs including soft-deleted insights. This authority predates the PR; pagination does not visibly expand it.

Trust Boundaries and Controls

  • observed — The production workflow constructs discovery inputs and forwards discovered IDs to migration. The inspected path adds no public request handler or new caller-controlled SQL fragment. Authorization for arbitrary Temporal task submission remains outside the established evidence.

Resilience and Maintainability Implications

  • inferred — The ID-window and per-call limits improve containment of discovery work under compatible execution. They are not a measured wall-clock or JSON-size budget, and the rollout compatibility concern can undermine termination despite bounded individual calls.

Hardening Proposals

  • proposed — Make producer-consumer compatibility explicit through a versioned activity contract or confirmed version-affine deployment. Validate legacy terminal results, new empty nonterminal results, replay, and rollback before relying on the completion signal.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the problem, changes, tests and rationale, release status, documentation, and agent context. It also explains the unmeasured production performance and the plan to check timing.…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant