perf(db): optimize scoped retrieval queries#714
Merged
Conversation
…igration-preflight-b463 # Conflicts: # docs/branch-review-ledger.md
|
Updates to Preview Branch (codex/chat-supabase-migration-preflight-b463) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Contributor
📝 WalkthroughWalkthroughChangesRetrieval optimization and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant service_role
participant v2_RPC
participant scoped_RPC
participant retrieval_tables
service_role->>v2_RPC: invoke retrieval
v2_RPC->>scoped_RPC: forward scope parameters
scoped_RPC->>retrieval_tables: filter and rank candidates
retrieval_tables-->>scoped_RPC: return matching rows
scoped_RPC-->>service_role: return scoped results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@scripts/generate-drift-manifest.ts`:
- Around line 62-70: Remove the preemptive docker(["rm", "-f", container])
cleanup around the scratch-container startup, including the corresponding
try/catch near the cleanup path. Let docker(["run", ...]) fail naturally when
the requested container name is already in use, and ensure later cleanup
executes only after this invocation successfully starts its own container.
In `@supabase/migrations/20260717160000_optimize_owner_public_retrieval.sql`:
- Around line 280-296: Apply the min_similarity threshold to the index-unit
retrieval query before candidate limiting, filtering on vector similarity so
low-similarity rows are excluded. Mirror this change in both
supabase/migrations/20260717160000_optimize_owner_public_retrieval.sql (lines
280-296) and supabase/schema.sql (lines 7245-7261), then add regression coverage
verifying candidates below the requested threshold are not returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 60a36258-e3a0-48e3-8e29-827e564e7b79
📒 Files selected for processing (6)
docs/branch-review-ledger.mdscripts/generate-drift-manifest.tssupabase/drift-manifest.jsonsupabase/migrations/20260717160000_optimize_owner_public_retrieval.sqlsupabase/schema.sqltests/retrieval-access-scope.test.ts
Comment on lines
+62
to
+70
| console.log(`Starting scratch container ${container} (${image}) on port ${port}…`); | ||
| try { | ||
| docker(["rm", "-f", CONTAINER]); | ||
| docker(["rm", "-f", container]); | ||
| } catch { | ||
| // not running — fine | ||
| } | ||
| const startedAt = Date.now(); | ||
| const scratchPassword = randomBytes(16).toString("hex"); | ||
| docker(["run", "-d", "--name", CONTAINER, "-e", `POSTGRES_PASSWORD=${scratchPassword}`, "-p", `${port}:5432`, image]); | ||
| docker(["run", "-d", "--name", container, "-e", `POSTGRES_PASSWORD=${scratchPassword}`, "-p", `${port}:5432`, image]); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not forcibly remove an arbitrary --container target.
A typo or reused name currently executes docker rm -f against an unrelated container. Let docker run fail on collision; cleanup will then only run after this invocation successfully starts its own container.
Proposed fix
console.log(`Starting scratch container ${container} (${image}) on port ${port}…`);
- try {
- docker(["rm", "-f", container]);
- } catch {
- // not running — fine
- }
const startedAt = Date.now();As per coding guidelines, “do not kill or modify another project's server.”
Also applies to: 124-129
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/generate-drift-manifest.ts` around lines 62 - 70, Remove the
preemptive docker(["rm", "-f", container]) cleanup around the scratch-container
startup, including the corresponding try/catch near the cleanup path. Let
docker(["run", ...]) fail naturally when the requested container name is already
in use, and ensure later cleanup executes only after this invocation
successfully starts its own container.
Source: Coding guidelines
Comment on lines
+280
to
+296
| similarity, | ||
| text_rank, | ||
| ( | ||
| (similarity * 0.52) | ||
| + (least(text_rank, 1) * 0.28) | ||
| + (quality_score * 0.12) | ||
| + (case when extraction_mode in ('model_heavy', 'hybrid') then 0.04 else 0 end) | ||
| + (case | ||
| when unit_type in ('askable_question', 'threshold', 'table_fact', 'table_threshold', 'visual_askable_question') then 0.04 | ||
| when unit_type in ('workflow_step', 'medication_monitoring', 'flowchart_step', 'diagram_decision', 'medication_chart_row', 'risk_matrix_cell') then 0.03 | ||
| else 0 | ||
| end) | ||
| )::double precision as hybrid_score, | ||
| metadata | ||
| from ranked | ||
| order by hybrid_score desc, id | ||
| limit match_count; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor min_similarity in both index-unit implementations.
Both functions accept the threshold but return candidates without applying it, allowing results below the caller’s requested similarity.
supabase/migrations/20260717160000_optimize_owner_public_retrieval.sql#L280-L296: filter by vector similarity before candidate limiting.supabase/schema.sql#L7245-L7261: mirror the same filter in the canonical schema.
Add regression coverage proving low-similarity candidates are excluded.
📍 Affects 2 files
supabase/migrations/20260717160000_optimize_owner_public_retrieval.sql#L280-L296(this comment)supabase/schema.sql#L7245-L7261
🤖 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 `@supabase/migrations/20260717160000_optimize_owner_public_retrieval.sql`
around lines 280 - 296, Apply the min_similarity threshold to the index-unit
retrieval query before candidate limiting, filtering on vector similarity so
low-similarity rows are excluded. Mirror this change in both
supabase/migrations/20260717160000_optimize_owner_public_retrieval.sql (lines
280-296) and supabase/schema.sql (lines 7245-7261), then add regression coverage
verifying candidates below the requested threshold are not returned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
The production review found a P2 latency hotspot: text retrieval took about 1.269 seconds and the daily index-unit snapshot averaged 3.673 seconds. The old wrappers executed owner and public paths separately, while the index-unit query combined two indexed predicates with a cross-index OR.
After deployment, warm text retrieval measured 34 ms and warm index-unit retrieval measured 36–39 ms. Access-scope parity remained unchanged.
Production and governance
Migration
20260717160000_optimize_owner_public_retrieval.sqlhas already been applied to Clinical KB Database (sjrfecxgysukkwxsowpy) through the linked migration workflow.No unexpected schema drift20260717160000Verification
Passed:
npm run check:function-grantsnpm run check:production-readinessnpm run check:supabase-projectnpm run check:driftnpx supabase migration list --linkedgit diff --checkThe exact integrated-head
verify:pr-localplan was attempted twice but did not start because another Database worktree held the repository-wide heavyweight test lock. Hosted CI is therefore the required exact-head merge gate. The earlier broadverify:cheaprun reached the 10-minute host timeout during aggregate Vitest; all affected focused and domain suites passed.Residual risk
Cold-cache index-unit retrieval remains sensitive to physical reads (2.376 seconds in the measured cold run). Backup/PITR restoration and write-load testing were not performed.
Summary by CodeRabbit
Performance
Security & Access
Reliability
Maintenance