Remove sensor priority tables from CDC and clean up legacy validation mirror - #1171
Merged
Merged
Conversation
The sensor_provider_priority and sensor_device_priority tables were added to buildPostgresFitnessRawTableStatements() in #1168 but no new ClickHouse migration was created, so existing environments (where migration 0008 already ran) never created these tables. The CDC step then fails with: Could not find table: sensor_provider_priority Add migration 0018 to create the missing tables.
sensor_provider_priority and sensor_device_priority are not read by any ClickHouse read model. Including them in the dofek_fitness_raw_analytics mirror mapping caused reconcileRawAnalyticsMirrors to drop and recreate the entire mirror on deploy, triggering a full initial snapshot that exhausted server resources and crashed production. Remove the two tables from: - analyticsSourceTables (Postgres publication) - rawAnalyticsMirrorTableMappings (reconcile check) - PeerDB SQL template (mirror definition) The ClickHouse raw tables (created by migration 0018) remain for future use but are not CDC-replicated.
The dofek_metric_stream_cdc mirror wrote to peerdb.metric_stream as a validation target during initial PeerDB rollout. Analytics now reads exclusively from postgres_fitness.metric_stream via the dofek_metric_stream_analytics mirror, making the validation mirror dead weight. Its replication slot was lost during the 2026-05-22 incident. Rather than recreating it (which would trigger a full metric_stream snapshot and crash the server again), remove it entirely: - Remove dofek_clickhouse peer (database=peerdb) from SQL template - Remove dofek_metric_stream_cdc mirror from SQL template - Remove CREATE DATABASE IF NOT EXISTS peerdb from CDC setup - Remove clickHouseDatabase from PeerDbSqlTemplateValues interface - Document mirror reconciliation behavior in ClickHouse docs - Update README and deploy docs to reflect current architecture
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
Reviewer's GuideRemoves unused sensor priority tables from ClickHouse CDC mirror mappings, deletes a legacy PeerDB validation mirror and its dedicated ClickHouse peer/database, adjusts CDC setup/tests accordingly, adds a migration for sensor priority raw tables, and updates documentation to reflect the current PeerDB architecture and mirror reconciliation behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR restructures the PeerDB CDC architecture by removing the intermediate validation mirror ( ChangesCDC Architecture Restructuring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The tests around
peerDbQueriesare quite index-sensitive (e.g., asserting specific items at positions 1–6); consider refactoring these to search by query content or labels to make them more robust to future peer/mirror reordering. - Given that
analyticsSourceTablesandrawAnalyticsMirrorTableMappings.dofek_fitness_raw_analyticsnow both hardcode the same table list minus provider inventory, it may be worth centralizing this list or deriving one from the other to avoid future drift when adding/removing tables. - Since mirror reconciliation can be destructive under resource constraints, you might consider adding a runtime guard (e.g., an explicit opt-in flag or dry-run mode) in
reconcileRawAnalyticsMirrorsinstead of relying solely on documentation to prevent accidental mirror-wide rebuilds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests around `peerDbQueries` are quite index-sensitive (e.g., asserting specific items at positions 1–6); consider refactoring these to search by query content or labels to make them more robust to future peer/mirror reordering.
- Given that `analyticsSourceTables` and `rawAnalyticsMirrorTableMappings.dofek_fitness_raw_analytics` now both hardcode the same table list minus provider inventory, it may be worth centralizing this list or deriving one from the other to avoid future drift when adding/removing tables.
- Since mirror reconciliation can be destructive under resource constraints, you might consider adding a runtime guard (e.g., an explicit opt-in flag or dry-run mode) in `reconcileRawAnalyticsMirrors` instead of relying solely on documentation to prevent accidental mirror-wide rebuilds.
## Individual Comments
### Comment 1
<location path="src/db/clickhouse-cdc.ts" line_range="467" />
<code_context>
export async function setupClickHouseCdc(options: SetupClickHouseCdcOptions): Promise<void> {
- await options.clickHouseClient.command({ query: "CREATE DATABASE IF NOT EXISTS peerdb" });
await ensureAnalyticsPeerDbColumns(options.clickHouseClient);
await ensureAnalyticsPublication(options.sourcePostgresClient);
await ensureMetricStreamNoImuPublication(options.sourcePostgresClient);
</code_context>
<issue_to_address>
**suggestion:** Re-evaluate whether `ensureAnalyticsPublication` is still needed now that the unfiltered metric_stream CDC mirror has been removed.
The SQL template now only uses the `peerdb_metric_stream_no_imu` publication for the analytics mirror; the old `peerdb_metric_stream_publication` used by `dofek_metric_stream_cdc` is no longer referenced. If that publication is deprecated, consider removing `ensureAnalyticsPublication` and any related publication-management logic to avoid unnecessary work on the source Postgres and ambiguity about which publication is active.
Suggested implementation:
```typescript
export async function setupClickHouseCdc(options: SetupClickHouseCdcOptions): Promise<void> {
await ensureAnalyticsPeerDbColumns(options.clickHouseClient);
await ensureMetricStreamNoImuPublication(options.sourcePostgresClient);
```
1. In `src/db/clickhouse-cdc.ts`, remove the import of `ensureAnalyticsPublication` (e.g. from a line like `import { ensureAnalyticsPeerDbColumns, ensureAnalyticsPublication, ensureMetricStreamNoImuPublication } from '...'`), keeping the other imports intact.
2. Locate the implementation of `ensureAnalyticsPublication` (likely in a helper module that manages Postgres publications) and:
- Remove the function entirely if `peerdb_metric_stream_publication` is no longer used anywhere.
- Delete any SQL or helper code that creates/updates/drops `peerdb_metric_stream_publication`.
3. Search the codebase for remaining references to `ensureAnalyticsPublication` and `peerdb_metric_stream_publication`:
- If `dofek_metric_stream_cdc` or any other code still depends on that publication, either update it to use `peerdb_metric_stream_no_imu` or keep the publication and document its purpose.
- If there are no remaining functional dependencies, remove those references to avoid dead code and ambiguity about which publication is active.
4. If there is configuration, migrations, or docs mentioning the old analytics publication, update or remove them to reflect that only `peerdb_metric_stream_no_imu` is now used for the analytics mirror.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The mock raw_analytics_mirror_config listed all current tables after sensor priority removal, so no mismatch was detected and the DROP never fired. Remove user_profile from the mock to create the expected mismatch.
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1171 and updates on each push. |
Asherlc
added a commit
that referenced
this pull request
May 22, 2026
…ror (#1172) * Truncate ClickHouse destination tables when reconciliation drops a CDC mirror When reconcileRawAnalyticsMirrors detects a table mapping mismatch and drops a mirror, the ClickHouse destination tables retain data from the previous initial copy. PeerDB then rejects mirror recreation with do_initial_copy=true because the destination tables are non-empty: table device_priority exists and is not empty Truncate the destination tables after dropping the mirror so the subsequent CREATE MIRROR IF NOT EXISTS succeeds. Fixes staging deploy failure in #1171. * Use Promise.all for parallel ClickHouse table truncation Address review feedback: tables are independent and can be truncated concurrently rather than sequentially.
This was referenced May 23, 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.
Root Cause
PR #1168 added
sensor_provider_priorityandsensor_device_priorityto the CDC mirror table mappings. On deploy,reconcileRawAnalyticsMirrorsdetected these tables were missing from the existing mirror config, dropped the entiredofek_fitness_raw_analyticsmirror, and recreated it withdo_initial_copy = true. This triggered a full initial snapshot of all 10 tables in that mirror, exhausting server resources and crashing production.The server crash also corrupted two other replication slots (
dofek_metric_stream_cdc,dofek_provider_inventory_raw_analytics), leaving them inloststate.Impact
Fix
Prevent recurrence
sensor_provider_priorityandsensor_device_priorityfrom CDC mirror mappings — no ClickHouse read model consumes them, so they should never have been in the mirror. The ClickHouse raw tables (created by migration 0018) remain for future use.Clean up legacy validation mirror
dofek_metric_stream_cdcmirror from PeerDB SQL template — it wrote topeerdb.metric_streamas a validation target during initial PeerDB rollout. Analytics reads exclusively frompostgres_fitness.metric_streamviadofek_metric_stream_analytics. The validation mirror was dead weight.dofek_clickhousepeer (database=peerdb) — only used by the removed mirror.CREATE DATABASE IF NOT EXISTS peerdbfrom CDC setup — no longer needed.clickHouseDatabasefromPeerDbSqlTemplateValuesinterface.Manual remediation (already done on production)
peerflow_slot_dofek_metric_stream_cdc,peerflow_slot_dofek_provider_inventory_raw_analyticsdofek_metric_stream_cdc,dofek_provider_inventory_raw_analyticsdofek_metric_stream_analyticsreconnected and is draining WAL lagDocs
docs/clickhouse-metric-stream.md: removedpeerdb.metric_streamreferences, added mirror reconciliation warningREADME.mdanddeploy/README.md: reflect current PeerDB architectureSummary by Sourcery
Remove unused sensor priority tables from PeerDB CDC mirror mappings and clean up legacy PeerDB validation infrastructure to prevent destructive mirror reinitialization.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Release Notes
Documentation
metric_streamand raw fitness tables into analytics read models for reporting.Chores