Add metric stream ID primary key migration - #1079
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSplit the metric_stream primary-key work into: migration 0007 (add id column + default and set replica identity at schema level) and migration 0009 (chunked backfill of NULL ids, set NOT NULL, add composite PK). Add idempotent oauth_token PK migration and update integration/unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Migration Runner
participant DB as Postgres DB
participant Chunks as TimescaleDB Chunks
Runner->>DB: apply 0007_metric_stream_primary_key.sql
DB-->>DB: ALTER TABLE add column `id` DEFAULT gen_random_uuid()
DB-->>DB: set relreplident = 'f' (replica identity full)
Runner->>DB: apply 0009_metric_stream_id_not_null_primary_key.sql
DB-->>DB: CREATE PROCEDURE backfill_metric_stream_ids(batch_size)
Runner->>DB: CALL backfill_metric_stream_ids()
DB->>DB: build temp table of chunk ranges
loop per chunk
DB->>Chunks: optionally decompress chunk (if compressed)
DB->>DB: UPDATE rows in chunk WHERE id IS NULL (batched)
DB->>Chunks: optionally recompress chunk
end
DB-->>DB: VERIFY no NULL ids remain
DB-->>DB: ALTER TABLE SET id NOT NULL
DB-->>DB: CREATE PRIMARY KEY (id, recorded_at)
Runner->>DB: apply 0010_oauth_token_primary_key.sql
DB-->>DB: IF no PK on oauth_token THEN add constraint using existing index
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 6 minutes and 18 seconds. Comment |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/db/metric-stream-replica-identity.integration.test.ts (1)
152-176: ⚡ Quick winAssert the chunk is still compressed after
0009.Right now this only proves that a compressed chunk exists before the backfill runs. If
0009decompresses a chunk and fails to restore its prior state, this test still passes. Add the sametimescaledb_information.chunksassertion after the secondrunMigrations(...)call so the compressed-chunk scenario is actually covered.Suggested assertion
const primaryKeyMigrationCount = await runMigrations(connectionString, tmpDir); expect(primaryKeyMigrationCount).toBe(1); + const recompressedChunkResult = await client.query<{ compressed_chunk_count: string }>(` + SELECT count(*) AS compressed_chunk_count + FROM timescaledb_information.chunks + WHERE hypertable_schema = 'fitness' + AND hypertable_name = 'metric_stream' + AND is_compressed + `); + expect(recompressedChunkResult.rows).toEqual([{ compressed_chunk_count: "1" }]); + const backfilledResult = await client.query<{ missing_id_count: string }>( "SELECT count(*) AS missing_id_count FROM fitness.metric_stream WHERE id IS NULL", );As per coding guidelines, "TDD: Write tests first, then implement. When fixing bugs, write a failing test that reproduces the bug before writing the fix."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/metric-stream-replica-identity.integration.test.ts` around lines 152 - 176, The test currently only checks timescaledb_information.chunks before running the primary-key migration; after calling runMigrations(connectionString, tmpDir) the test should re-query timescaledb_information.chunks to assert the compressed chunk still exists (e.g., repeat the client.query SELECT count(*) ... WHERE is_compressed and expect the count to remain "1") so that runMigrations/0009_metric_stream_id_not_null_primary_key.sql does not inadvertently decompress chunks; place this new assertion immediately after the primaryKeyMigrationCount(expect) block and before verifying no NULL ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0009_metric_stream_id_not_null_primary_key.sql`:
- Around line 43-50: The current temp table metric_stream_backfill_chunks must
capture each chunk's original compression state from
timescaledb_information.chunks (e.g., add is_compressed boolean) and use that
when deciding to recompress; modify the SELECT that builds
metric_stream_backfill_chunks to include is_compressed, ensure the migration
calls decompress_chunk(...) for all chunks to allow backfill but only calls
compress_chunk(...) for rows where is_compressed = true AND
should_compress_after is true, so chunks that were originally uncompressed
remain uncompressed and originally compressed chunks are restored if old enough.
- Around line 177-178: The direct ALTER TABLE metric_stream ALTER COLUMN id SET
NOT NULL will take an ACCESS EXCLUSIVE lock and perform a full scan; instead add
a NOT VALID check constraint, validate it, then set NOT NULL. Concretely: add a
constraint like metric_stream_id_not_null_chk as CHECK (id IS NOT NULL) NOT
VALID on table metric_stream, run ALTER TABLE metric_stream VALIDATE CONSTRAINT
metric_stream_id_not_null_chk to ensure there are no NULLs (this avoids the full
scan under exclusive lock), and only after validation run ALTER TABLE
metric_stream ALTER COLUMN id SET NOT NULL and optionally DROP CONSTRAINT
metric_stream_id_not_null_chk if you don’t want the duplicate constraint.
---
Nitpick comments:
In `@src/db/metric-stream-replica-identity.integration.test.ts`:
- Around line 152-176: The test currently only checks
timescaledb_information.chunks before running the primary-key migration; after
calling runMigrations(connectionString, tmpDir) the test should re-query
timescaledb_information.chunks to assert the compressed chunk still exists
(e.g., repeat the client.query SELECT count(*) ... WHERE is_compressed and
expect the count to remain "1") so that
runMigrations/0009_metric_stream_id_not_null_primary_key.sql does not
inadvertently decompress chunks; place this new assertion immediately after the
primaryKeyMigrationCount(expect) block and before verifying no NULL ids.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a754879-a160-42ca-b54e-f8bbd5f5426a
📒 Files selected for processing (7)
drizzle/0007_metric_stream_primary_key.sqldrizzle/0009_metric_stream_id_not_null_primary_key.sqldrizzle/0010_oauth_token_primary_key.sqlsrc/db/metric-stream-primary-key.integration.test.tssrc/db/metric-stream-replica-identity.integration.test.tssrc/db/migrate.integration.test.tssrc/db/migrate.test.ts
💤 Files with no reviewable changes (2)
- src/db/metric-stream-primary-key.integration.test.ts
- drizzle/0007_metric_stream_primary_key.sql
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the database migration sequence to safely introduce a primary key for the TimescaleDB fitness.metric_stream hypertable by separating replica identity changes from a chunk-aware ID backfill, and adds a primary key to fitness.oauth_token using an existing unique index.
Changes:
- Split
metric_streamwork into: (1) addiddefault +REPLICA IDENTITY FULL, then (2) chunk-aware backfill + enforceid NOT NULL+ add(id, recorded_at)primary key. - Add
oauth_tokencomposite primary key via existing(user_id, provider_id)unique index. - Update/replace integration tests to cover compressed chunks, replica identity behavior, and missing primary keys.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/db/migrate.test.ts | Adds a unit test to ensure migration parsing/execution doesn’t apply custom marker “hooks”. |
| src/db/migrate.integration.test.ts | Updates assertions for metric_stream replica identity + PK columns and adds coverage for oauth_token PK. |
| src/db/metric-stream-replica-identity.integration.test.ts | New end-to-end repro that validates the two-step metric_stream migration behavior, including compressed chunks. |
| src/db/metric-stream-primary-key.integration.test.ts | Removes the older integration test in favor of the more complete new repro. |
| drizzle/0007_metric_stream_primary_key.sql | Narrows the earlier migration to only set default id + REPLICA IDENTITY FULL (no backfill/PK yet). |
| drizzle/0009_metric_stream_id_not_null_primary_key.sql | Adds the chunk-aware backfill procedure, enforces id NOT NULL, and creates the hypertable-compatible PK. |
| drizzle/0010_oauth_token_primary_key.sql | Adds oauth_token PK using the existing unique index and enforces user_id NOT NULL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/db/metric-stream-replica-identity.integration.test.ts (1)
156-199: ⚡ Quick winAssert compression state per chunk, not just the total count.
compressed_chunk_count = "1"still passes if0009decompresses the original chunk and recompresses a different one. Snapshotchunk_name/is_compressedbefore applying0009and compare the same ordered rows afterward so this test catches chunk-level compression drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/metric-stream-replica-identity.integration.test.ts` around lines 156 - 199, Snapshot the per-chunk compression state (select chunk_name and is_compressed ordered deterministically) before applying migration and store it (e.g., replace chunkStateResult with rows containing chunk_name and is_compressed from the timescaledb_information.chunks query), then after runMigrations (replace finalChunkStateResult assertion) run the same ordered query and assert the two row arrays are equal; reference the existing queries around chunkStateResult and finalChunkStateResult and keep the filtering WHERE hypertable_schema = 'fitness' AND hypertable_name = 'metric_stream' and ordering (e.g., ORDER BY chunk_name) to ensure chunk-level compression drift is detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0009_metric_stream_id_not_null_primary_key.sql`:
- Around line 25-28: The procedure's main BEGIN lacks an EXCEPTION handler so
session settings and chunk state may be left altered on failure; add an
EXCEPTION WHEN OTHERS block around the main BEGIN...END that restores prior
session configs (reset statement_timeout, lock_timeout,
timescaledb.max_tuples_decompressed_per_dml_transaction and
session_replication_role using the values saved earlier) and, if
should_recompress is true and current_chunk_regclass was decompressed, call
compress_chunk(current_chunk_regclass) (or the equivalent recompression command)
to recompress the chunk, then RAISE the exception to propagate it; reference the
existing variables and calls like current_chunk_regclass, should_recompress,
decompress_chunk, and the saved config variables to implement the
restore/recompress logic.
---
Nitpick comments:
In `@src/db/metric-stream-replica-identity.integration.test.ts`:
- Around line 156-199: Snapshot the per-chunk compression state (select
chunk_name and is_compressed ordered deterministically) before applying
migration and store it (e.g., replace chunkStateResult with rows containing
chunk_name and is_compressed from the timescaledb_information.chunks query),
then after runMigrations (replace finalChunkStateResult assertion) run the same
ordered query and assert the two row arrays are equal; reference the existing
queries around chunkStateResult and finalChunkStateResult and keep the filtering
WHERE hypertable_schema = 'fitness' AND hypertable_name = 'metric_stream' and
ordering (e.g., ORDER BY chunk_name) to ensure chunk-level compression drift is
detected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b1c778b-fde3-4e21-9714-db3c79b84db9
📒 Files selected for processing (4)
drizzle/0009_metric_stream_id_not_null_primary_key.sqldrizzle/0010_oauth_token_primary_key.sqlsrc/db/metric-stream-replica-identity.integration.test.tssrc/db/migrate.integration.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- drizzle/0010_oauth_token_primary_key.sql
- src/db/migrate.integration.test.ts
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1079 and updates on each push. |
This was referenced May 1, 2026
This was referenced Jun 7, 2026
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
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
metric_streammigration to add onlyiddefault plusREPLICA IDENTITY FULLmetric_stream.idbackfill, enforceid NOT NULL, and create the hypertable-compatible primary key on(id, recorded_at)fitness.oauth_tokenusing the existing unique user/provider indexValidation
pnpm vitest run src/db/migrate.test.ts src/db/migrate.integration.test.ts src/db/metric-stream-replica-identity.integration.test.tspnpm lintpnpm tsc --noEmitcd packages/server && pnpm tsc --noEmitcd packages/web && pnpm tsc --noEmitpnpm test:changedTEST_DATABASE_URL=postgres://health:health@127.0.0.1:5435/health pnpm testNote: the first full
pnpm testattempt hit a local Testcontainers startup failure (No host port found for host IP) infatsecret-sync.integration.test.ts. That suite passed in isolation, then the full suite passed against the shared local Timescale URL above.Summary by CodeRabbit
Database Schema Updates
Testing