fix: gate matview read path on shape check to prevent "column does not exist" during rolling deploys - #5384
Merged
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMaterialized-view readiness
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PostgresLogStore
participant EnsureMatViews
participant ShapeCheck
participant Refresher
PostgresLogStore->>EnsureMatViews: maintain materialized views
EnsureMatViews-->>PostgresLogStore: ownership and error
PostgresLogStore->>ShapeCheck: validate shapes when not owner
ShapeCheck-->>PostgresLogStore: ready status
PostgresLogStore->>Refresher: start periodic refresh
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
impoiler
changed the title
impoiler
force-pushed
the
07-20-fix_matview_ready_state
branch
from
July 20, 2026 14:18
9df18eb to
1fda164
Compare
impoiler
marked this pull request as ready for review
July 20, 2026 14:36
Contributor
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix: matview ready state" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/logstore/matviews.go (1)
488-551: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding direct test coverage for
matViewShapesReady/pqTextArray.The ownership contract (
ensureMatViews) is covered bymatviews_lock_test.go, butmatViewShapesReady— the function that actually gates enabling the read path when maintenance is owned by another replica — doesn't appear to have a dedicated test in this diff (e.g., create an old-shape matview missing a required column, assertfalse; then create the correct shape and asserttrue). Given this function directly controls whether a rolling-deploy replica serves matview reads before the shape is confirmed, a focused test would materially reduce regression risk. The existingsetupPerfTestDB/testMatViewExistsharness inrdb_postgres_perf_test.gocould be reused for this.🤖 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 `@framework/logstore/matviews.go` around lines 488 - 551, Add focused tests for matViewShapesReady using the existing PostgreSQL test harness: verify it returns false for a managed materialized view with a missing required column, then true after the view has the complete required shape. Also add direct coverage for pqTextArray, including representative strings requiring escaping, while preserving existing behavior for non-Postgres databases and query errors.
🤖 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.
Nitpick comments:
In `@framework/logstore/matviews.go`:
- Around line 488-551: Add focused tests for matViewShapesReady using the
existing PostgreSQL test harness: verify it returns false for a managed
materialized view with a missing required column, then true after the view has
the complete required shape. Also add direct coverage for pqTextArray, including
representative strings requiring escaping, while preserving existing behavior
for non-Postgres databases and query errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cebd5fb-9c03-48fe-8ff3-0cddb8de8a3d
📒 Files selected for processing (5)
framework/logstore/matviews.goframework/logstore/matviews_lock_test.goframework/logstore/migrations_scale_test.goframework/logstore/postgres.goframework/logstore/rdb_postgres_perf_test.go
impoiler
force-pushed
the
07-20-fix_matview_ready_state
branch
2 times, most recently
from
July 20, 2026 15:16
7ea3aad to
58a5441
Compare
17 tasks
impoiler
force-pushed
the
07-20-fix_matview_ready_state
branch
from
July 21, 2026 03:26
58a5441 to
a464240
Compare
Pratham-Mishra04
approved these changes
Jul 21, 2026
Collaborator
Merge activity
|
Pratham-Mishra04
force-pushed
the
07-20-fix_matview_ready_state
branch
from
July 21, 2026 07:38
a464240 to
c6347af
Compare
17 tasks
akshaydeo
added a commit
that referenced
this pull request
Jul 23, 2026
## Summary This PR adds two improvements to the materialized view read path: cache-hit statistics are now served from the hybrid aggregate (interior buckets from `mv_logs_hourly`, boundary slivers classified raw) instead of a separate full-window raw scan, and a runtime self-heal mechanism automatically recovers from missing or stale-shaped materialized views without requiring a process restart. ## Changes - **Cache hits in the hybrid aggregate**: Three new columns (`direct_cache_hits`, `semantic_cache_hits`, `cache_debug_count`) are added to `mv_logs_hourly` using the same `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr` expressions shared across the matview DDL, boundary sliver queries, and `aggregateCacheHits`. `cache_debug_count` preserves the nil contract: when no row in the window carried valid `cache_debug` JSON the fields are omitted from the response, and when cache rows exist but none were direct/semantic explicit zeros are returned. The previous approach issued a separate full-window raw scan for cache hits after the hybrid aggregate completed; that scan is removed. - **Runtime matview self-heal** (`matviewheal.go`): `isMatViewShapeError` classifies PostgreSQL error codes `42P01` (undefined table), `42703` (undefined column), and `55000` (object not in prerequisite state) as shape errors. `fallBackToRaw` is called at every matview dispatch site — on a shape error it disables the matview read path process-wide, logs a warning, and triggers a single-flight background repair via `triggerMatViewSelfHeal`. The repair runs `ensureMatViews` then `refreshMatViews` and re-enables the path on success. A 30-second cooldown (`matViewHealCooldown`) bounds repair frequency; while broken, every request continues succeeding via the raw fallback. Two new atomic fields (`matViewHealInFlight`, `matViewHealLastAttempt`) are added to `RDBLogStore`. - **Shared SQL constants**: The inline regex strings for the cache debug guard and hit-type extractor are replaced with named constants `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr`, used consistently across the matview DDL, `applyFilters`, `rawTerminalStatsAgg`, and `aggregateCacheHits`. - **Tests**: `TestGetStatsMatViewCacheHitsHybrid` verifies the hybrid cache-hit path including nil and zero contracts and agreement with the raw path. `TestMatViewShapeErrorFallsBackAndSelfHeals`, `TestMatViewStaleShapeFallsBackAndSelfHeals`, and `TestFilterMatViewShapeErrorFallsBack` cover the 42P01, 42703, and filter-view drop scenarios end-to-end, including background self-heal convergence. ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/logstore/... -run TestGetStatsMatViewCacheHitsHybrid go test ./framework/logstore/... -run TestMatViewShapeErrorFallsBackAndSelfHeals go test ./framework/logstore/... -run TestMatViewStaleShapeFallsBackAndSelfHeals go test ./framework/logstore/... -run TestFilterMatViewShapeErrorFallsBack go test ./framework/logstore/... -run TestIsMatViewShapeError go test ./framework/logstore/... ``` The self-heal tests drop or replace `mv_logs_hourly` mid-run and assert that reads continue returning correct results from the raw table with no error, that `matViewsReady` is set to false immediately, and that the view is recreated with the correct shape within 90 seconds. ## Breaking changes - [x] No The new matview columns require a schema migration. On first deploy, `repairMatViewShapes` detects the missing columns, drops and recreates `mv_logs_hourly`, and the self-heal path handles any replica that reads before the rebuild completes. ## Related issues Closes #5384 ## Security considerations No auth, secrets, PII, or sandboxing changes. The new SQL expressions are constants composed only of built-in PostgreSQL operators and are not user-controlled. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
18 tasks
akshaydeo
added a commit
that referenced
this pull request
Jul 24, 2026
## Summary This PR adds two improvements to the materialized view read path: cache-hit statistics are now served from the hybrid aggregate (interior buckets from `mv_logs_hourly`, boundary slivers classified raw) instead of a separate full-window raw scan, and a runtime self-heal mechanism automatically recovers from missing or stale-shaped materialized views without requiring a process restart. ## Changes - **Cache hits in the hybrid aggregate**: Three new columns (`direct_cache_hits`, `semantic_cache_hits`, `cache_debug_count`) are added to `mv_logs_hourly` using the same `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr` expressions shared across the matview DDL, boundary sliver queries, and `aggregateCacheHits`. `cache_debug_count` preserves the nil contract: when no row in the window carried valid `cache_debug` JSON the fields are omitted from the response, and when cache rows exist but none were direct/semantic explicit zeros are returned. The previous approach issued a separate full-window raw scan for cache hits after the hybrid aggregate completed; that scan is removed. - **Runtime matview self-heal** (`matviewheal.go`): `isMatViewShapeError` classifies PostgreSQL error codes `42P01` (undefined table), `42703` (undefined column), and `55000` (object not in prerequisite state) as shape errors. `fallBackToRaw` is called at every matview dispatch site — on a shape error it disables the matview read path process-wide, logs a warning, and triggers a single-flight background repair via `triggerMatViewSelfHeal`. The repair runs `ensureMatViews` then `refreshMatViews` and re-enables the path on success. A 30-second cooldown (`matViewHealCooldown`) bounds repair frequency; while broken, every request continues succeeding via the raw fallback. Two new atomic fields (`matViewHealInFlight`, `matViewHealLastAttempt`) are added to `RDBLogStore`. - **Shared SQL constants**: The inline regex strings for the cache debug guard and hit-type extractor are replaced with named constants `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr`, used consistently across the matview DDL, `applyFilters`, `rawTerminalStatsAgg`, and `aggregateCacheHits`. - **Tests**: `TestGetStatsMatViewCacheHitsHybrid` verifies the hybrid cache-hit path including nil and zero contracts and agreement with the raw path. `TestMatViewShapeErrorFallsBackAndSelfHeals`, `TestMatViewStaleShapeFallsBackAndSelfHeals`, and `TestFilterMatViewShapeErrorFallsBack` cover the 42P01, 42703, and filter-view drop scenarios end-to-end, including background self-heal convergence. ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/logstore/... -run TestGetStatsMatViewCacheHitsHybrid go test ./framework/logstore/... -run TestMatViewShapeErrorFallsBackAndSelfHeals go test ./framework/logstore/... -run TestMatViewStaleShapeFallsBackAndSelfHeals go test ./framework/logstore/... -run TestFilterMatViewShapeErrorFallsBack go test ./framework/logstore/... -run TestIsMatViewShapeError go test ./framework/logstore/... ``` The self-heal tests drop or replace `mv_logs_hourly` mid-run and assert that reads continue returning correct results from the raw table with no error, that `matViewsReady` is set to false immediately, and that the view is recreated with the correct shape within 90 seconds. ## Breaking changes - [x] No The new matview columns require a schema migration. On first deploy, `repairMatViewShapes` detects the missing columns, drops and recreates `mv_logs_hourly`, and the self-heal path handles any replica that reads before the rebuild completes. ## Related issues Closes #5384 ## Security considerations No auth, secrets, PII, or sandboxing changes. The new SQL expressions are constants composed only of built-in PostgreSQL operators and are not user-controlled. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
BearTS
pushed a commit
that referenced
this pull request
Jul 27, 2026
This PR adds two improvements to the materialized view read path: cache-hit statistics are now served from the hybrid aggregate (interior buckets from `mv_logs_hourly`, boundary slivers classified raw) instead of a separate full-window raw scan, and a runtime self-heal mechanism automatically recovers from missing or stale-shaped materialized views without requiring a process restart. - **Cache hits in the hybrid aggregate**: Three new columns (`direct_cache_hits`, `semantic_cache_hits`, `cache_debug_count`) are added to `mv_logs_hourly` using the same `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr` expressions shared across the matview DDL, boundary sliver queries, and `aggregateCacheHits`. `cache_debug_count` preserves the nil contract: when no row in the window carried valid `cache_debug` JSON the fields are omitted from the response, and when cache rows exist but none were direct/semantic explicit zeros are returned. The previous approach issued a separate full-window raw scan for cache hits after the hybrid aggregate completed; that scan is removed. - **Runtime matview self-heal** (`matviewheal.go`): `isMatViewShapeError` classifies PostgreSQL error codes `42P01` (undefined table), `42703` (undefined column), and `55000` (object not in prerequisite state) as shape errors. `fallBackToRaw` is called at every matview dispatch site — on a shape error it disables the matview read path process-wide, logs a warning, and triggers a single-flight background repair via `triggerMatViewSelfHeal`. The repair runs `ensureMatViews` then `refreshMatViews` and re-enables the path on success. A 30-second cooldown (`matViewHealCooldown`) bounds repair frequency; while broken, every request continues succeeding via the raw fallback. Two new atomic fields (`matViewHealInFlight`, `matViewHealLastAttempt`) are added to `RDBLogStore`. - **Shared SQL constants**: The inline regex strings for the cache debug guard and hit-type extractor are replaced with named constants `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr`, used consistently across the matview DDL, `applyFilters`, `rawTerminalStatsAgg`, and `aggregateCacheHits`. - **Tests**: `TestGetStatsMatViewCacheHitsHybrid` verifies the hybrid cache-hit path including nil and zero contracts and agreement with the raw path. `TestMatViewShapeErrorFallsBackAndSelfHeals`, `TestMatViewStaleShapeFallsBackAndSelfHeals`, and `TestFilterMatViewShapeErrorFallsBack` cover the 42P01, 42703, and filter-view drop scenarios end-to-end, including background self-heal convergence. - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ```sh go test ./framework/logstore/... -run TestGetStatsMatViewCacheHitsHybrid go test ./framework/logstore/... -run TestMatViewShapeErrorFallsBackAndSelfHeals go test ./framework/logstore/... -run TestMatViewStaleShapeFallsBackAndSelfHeals go test ./framework/logstore/... -run TestFilterMatViewShapeErrorFallsBack go test ./framework/logstore/... -run TestIsMatViewShapeError go test ./framework/logstore/... ``` The self-heal tests drop or replace `mv_logs_hourly` mid-run and assert that reads continue returning correct results from the raw table with no error, that `matViewsReady` is set to false immediately, and that the view is recreated with the correct shape within 90 seconds. - [x] No The new matview columns require a schema migration. On first deploy, `repairMatViewShapes` detects the missing columns, drops and recreates `mv_logs_hourly`, and the self-heal path handles any replica that reads before the rebuild completes. Closes #5384 No auth, secrets, PII, or sandboxing changes. The new SQL expressions are constants composed only of built-in PostgreSQL operators and are not user-controlled. - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
This was referenced Jul 28, 2026
This was referenced Jul 30, 2026
Merged
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
…t exist" during rolling deploys (maximhq#5384) ## Summary During a rolling deploy, a replica that loses the `ensureMatViews` advisory lock race could enable the matview read path while the lock-holding replica was still mid-repair. This caused queries like `getModelHistogramFromMatView` to hit columns (`cancelled_count`, `throughput_*`) that the old-schema view did not yet have, producing "column does not exist" errors. ## Changes - `ensureMatViews` now returns `(bool, error)` — the bool indicates whether this process actually performed the create/repair work. A `false` return with a `nil` error means another replica held the lock; callers must not treat this as "views are usable." - Added `matViewShapesReady`, a read-only `pg_catalog` check that verifies every managed matview exists and carries the full column set this build requires. It is safe and cheap to call from replicas that skipped the lock, and is used as the gate before enabling the matview read path. - Added `pqTextArray`, a minimal helper that renders a `[]string` as a Postgres `text[]` literal for use as a query parameter, avoiding a driver-specific array type dependency. Backslashes and double-quotes are escaped correctly. - In `newPostgresLogStore`, when `ensureMatViews` returns `false` (lock held elsewhere), the startup path now calls `matViewShapesReady` before enabling the read path. If the shapes are not current yet, it starts the refresher's recovery path instead of flipping `matViewsReady` immediately. - In `startMatViewRefresher`, a successful `refreshMatViews` tick no longer unconditionally flips `matViewsReady`. It now calls `matViewShapesReady` first, since `REFRESH` succeeds on an old-shape view and `refreshMatViews` also returns `nil` when it skipped due to the lock or activity gate. - Added `pg_catalog.pg_table_is_visible` filters to the existence and column-listing queries in `matViewNeedsRebuild` to avoid matching same-named views in other schemas. - Updated all call sites and tests to handle the new `(bool, error)` signature. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) ## How to test ```sh go test ./framework/logstore/... ``` The existing `TestEnsureMatViewsSharesRefreshAdvisoryLock` test now asserts the `bool` return value in both the lock-held and lock-free cases. The scale migration test and perf tests have been updated to match the new signature. To validate the rolling-deploy scenario specifically: run two instances sharing the same Postgres database against a schema that is missing the new matview columns, confirm that the replica that loses the lock race does not enable the matview read path until `matViewShapesReady` returns true. ## Breaking changes - [x] No Internal function signature change only (`ensureMatViews` return type). No exported API is affected. ## Security considerations `pqTextArray` escapes backslashes and double-quotes. Column names passed to it come from package-level constants, not user input, but the escaping is correct regardless. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
…hq#5507) ## Summary This PR adds two improvements to the materialized view read path: cache-hit statistics are now served from the hybrid aggregate (interior buckets from `mv_logs_hourly`, boundary slivers classified raw) instead of a separate full-window raw scan, and a runtime self-heal mechanism automatically recovers from missing or stale-shaped materialized views without requiring a process restart. ## Changes - **Cache hits in the hybrid aggregate**: Three new columns (`direct_cache_hits`, `semantic_cache_hits`, `cache_debug_count`) are added to `mv_logs_hourly` using the same `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr` expressions shared across the matview DDL, boundary sliver queries, and `aggregateCacheHits`. `cache_debug_count` preserves the nil contract: when no row in the window carried valid `cache_debug` JSON the fields are omitted from the response, and when cache rows exist but none were direct/semantic explicit zeros are returned. The previous approach issued a separate full-window raw scan for cache hits after the hybrid aggregate completed; that scan is removed. - **Runtime matview self-heal** (`matviewheal.go`): `isMatViewShapeError` classifies PostgreSQL error codes `42P01` (undefined table), `42703` (undefined column), and `55000` (object not in prerequisite state) as shape errors. `fallBackToRaw` is called at every matview dispatch site — on a shape error it disables the matview read path process-wide, logs a warning, and triggers a single-flight background repair via `triggerMatViewSelfHeal`. The repair runs `ensureMatViews` then `refreshMatViews` and re-enables the path on success. A 30-second cooldown (`matViewHealCooldown`) bounds repair frequency; while broken, every request continues succeeding via the raw fallback. Two new atomic fields (`matViewHealInFlight`, `matViewHealLastAttempt`) are added to `RDBLogStore`. - **Shared SQL constants**: The inline regex strings for the cache debug guard and hit-type extractor are replaced with named constants `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr`, used consistently across the matview DDL, `applyFilters`, `rawTerminalStatsAgg`, and `aggregateCacheHits`. - **Tests**: `TestGetStatsMatViewCacheHitsHybrid` verifies the hybrid cache-hit path including nil and zero contracts and agreement with the raw path. `TestMatViewShapeErrorFallsBackAndSelfHeals`, `TestMatViewStaleShapeFallsBackAndSelfHeals`, and `TestFilterMatViewShapeErrorFallsBack` cover the 42P01, 42703, and filter-view drop scenarios end-to-end, including background self-heal convergence. ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/logstore/... -run TestGetStatsMatViewCacheHitsHybrid go test ./framework/logstore/... -run TestMatViewShapeErrorFallsBackAndSelfHeals go test ./framework/logstore/... -run TestMatViewStaleShapeFallsBackAndSelfHeals go test ./framework/logstore/... -run TestFilterMatViewShapeErrorFallsBack go test ./framework/logstore/... -run TestIsMatViewShapeError go test ./framework/logstore/... ``` The self-heal tests drop or replace `mv_logs_hourly` mid-run and assert that reads continue returning correct results from the raw table with no error, that `matViewsReady` is set to false immediately, and that the view is recreated with the correct shape within 90 seconds. ## Breaking changes - [x] No The new matview columns require a schema migration. On first deploy, `repairMatViewShapes` detects the missing columns, drops and recreates `mv_logs_hourly`, and the self-heal path handles any replica that reads before the rebuild completes. ## Related issues Closes maximhq#5384 ## Security considerations No auth, secrets, PII, or sandboxing changes. The new SQL expressions are constants composed only of built-in PostgreSQL operators and are not user-controlled. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
…t exist" during rolling deploys (maximhq#5384) ## Summary During a rolling deploy, a replica that loses the `ensureMatViews` advisory lock race could enable the matview read path while the lock-holding replica was still mid-repair. This caused queries like `getModelHistogramFromMatView` to hit columns (`cancelled_count`, `throughput_*`) that the old-schema view did not yet have, producing "column does not exist" errors. ## Changes - `ensureMatViews` now returns `(bool, error)` — the bool indicates whether this process actually performed the create/repair work. A `false` return with a `nil` error means another replica held the lock; callers must not treat this as "views are usable." - Added `matViewShapesReady`, a read-only `pg_catalog` check that verifies every managed matview exists and carries the full column set this build requires. It is safe and cheap to call from replicas that skipped the lock, and is used as the gate before enabling the matview read path. - Added `pqTextArray`, a minimal helper that renders a `[]string` as a Postgres `text[]` literal for use as a query parameter, avoiding a driver-specific array type dependency. Backslashes and double-quotes are escaped correctly. - In `newPostgresLogStore`, when `ensureMatViews` returns `false` (lock held elsewhere), the startup path now calls `matViewShapesReady` before enabling the read path. If the shapes are not current yet, it starts the refresher's recovery path instead of flipping `matViewsReady` immediately. - In `startMatViewRefresher`, a successful `refreshMatViews` tick no longer unconditionally flips `matViewsReady`. It now calls `matViewShapesReady` first, since `REFRESH` succeeds on an old-shape view and `refreshMatViews` also returns `nil` when it skipped due to the lock or activity gate. - Added `pg_catalog.pg_table_is_visible` filters to the existence and column-listing queries in `matViewNeedsRebuild` to avoid matching same-named views in other schemas. - Updated all call sites and tests to handle the new `(bool, error)` signature. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) ## How to test ```sh go test ./framework/logstore/... ``` The existing `TestEnsureMatViewsSharesRefreshAdvisoryLock` test now asserts the `bool` return value in both the lock-held and lock-free cases. The scale migration test and perf tests have been updated to match the new signature. To validate the rolling-deploy scenario specifically: run two instances sharing the same Postgres database against a schema that is missing the new matview columns, confirm that the replica that loses the lock race does not enable the matview read path until `matViewShapesReady` returns true. ## Breaking changes - [x] No Internal function signature change only (`ensureMatViews` return type). No exported API is affected. ## Security considerations `pqTextArray` escapes backslashes and double-quotes. Column names passed to it come from package-level constants, not user input, but the escaping is correct regardless. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
…hq#5507) ## Summary This PR adds two improvements to the materialized view read path: cache-hit statistics are now served from the hybrid aggregate (interior buckets from `mv_logs_hourly`, boundary slivers classified raw) instead of a separate full-window raw scan, and a runtime self-heal mechanism automatically recovers from missing or stale-shaped materialized views without requiring a process restart. ## Changes - **Cache hits in the hybrid aggregate**: Three new columns (`direct_cache_hits`, `semantic_cache_hits`, `cache_debug_count`) are added to `mv_logs_hourly` using the same `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr` expressions shared across the matview DDL, boundary sliver queries, and `aggregateCacheHits`. `cache_debug_count` preserves the nil contract: when no row in the window carried valid `cache_debug` JSON the fields are omitted from the response, and when cache rows exist but none were direct/semantic explicit zeros are returned. The previous approach issued a separate full-window raw scan for cache hits after the hybrid aggregate completed; that scan is removed. - **Runtime matview self-heal** (`matviewheal.go`): `isMatViewShapeError` classifies PostgreSQL error codes `42P01` (undefined table), `42703` (undefined column), and `55000` (object not in prerequisite state) as shape errors. `fallBackToRaw` is called at every matview dispatch site — on a shape error it disables the matview read path process-wide, logs a warning, and triggers a single-flight background repair via `triggerMatViewSelfHeal`. The repair runs `ensureMatViews` then `refreshMatViews` and re-enables the path on success. A 30-second cooldown (`matViewHealCooldown`) bounds repair frequency; while broken, every request continues succeeding via the raw fallback. Two new atomic fields (`matViewHealInFlight`, `matViewHealLastAttempt`) are added to `RDBLogStore`. - **Shared SQL constants**: The inline regex strings for the cache debug guard and hit-type extractor are replaced with named constants `cacheDebugJSONGuard` and `cacheDebugHitTypeExpr`, used consistently across the matview DDL, `applyFilters`, `rawTerminalStatsAgg`, and `aggregateCacheHits`. - **Tests**: `TestGetStatsMatViewCacheHitsHybrid` verifies the hybrid cache-hit path including nil and zero contracts and agreement with the raw path. `TestMatViewShapeErrorFallsBackAndSelfHeals`, `TestMatViewStaleShapeFallsBackAndSelfHeals`, and `TestFilterMatViewShapeErrorFallsBack` cover the 42P01, 42703, and filter-view drop scenarios end-to-end, including background self-heal convergence. ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/logstore/... -run TestGetStatsMatViewCacheHitsHybrid go test ./framework/logstore/... -run TestMatViewShapeErrorFallsBackAndSelfHeals go test ./framework/logstore/... -run TestMatViewStaleShapeFallsBackAndSelfHeals go test ./framework/logstore/... -run TestFilterMatViewShapeErrorFallsBack go test ./framework/logstore/... -run TestIsMatViewShapeError go test ./framework/logstore/... ``` The self-heal tests drop or replace `mv_logs_hourly` mid-run and assert that reads continue returning correct results from the raw table with no error, that `matViewsReady` is set to false immediately, and that the view is recreated with the correct shape within 90 seconds. ## Breaking changes - [x] No The new matview columns require a schema migration. On first deploy, `repairMatViewShapes` detects the missing columns, drops and recreates `mv_logs_hourly`, and the self-heal path handles any replica that reads before the rebuild completes. ## Related issues Closes maximhq#5384 ## Security considerations No auth, secrets, PII, or sandboxing changes. The new SQL expressions are constants composed only of built-in PostgreSQL operators and are not user-controlled. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
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
2 participants
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
During a rolling deploy, a replica that loses the
ensureMatViewsadvisory lock race could enable the matview read path while the lock-holding replica was still mid-repair. This caused queries likegetModelHistogramFromMatViewto hit columns (cancelled_count,throughput_*) that the old-schema view did not yet have, producing "column does not exist" errors.Changes
ensureMatViewsnow returns(bool, error)— the bool indicates whether this process actually performed the create/repair work. Afalsereturn with anilerror means another replica held the lock; callers must not treat this as "views are usable."matViewShapesReady, a read-onlypg_catalogcheck that verifies every managed matview exists and carries the full column set this build requires. It is safe and cheap to call from replicas that skipped the lock, and is used as the gate before enabling the matview read path.pqTextArray, a minimal helper that renders a[]stringas a Postgrestext[]literal for use as a query parameter, avoiding a driver-specific array type dependency. Backslashes and double-quotes are escaped correctly.newPostgresLogStore, whenensureMatViewsreturnsfalse(lock held elsewhere), the startup path now callsmatViewShapesReadybefore enabling the read path. If the shapes are not current yet, it starts the refresher's recovery path instead of flippingmatViewsReadyimmediately.startMatViewRefresher, a successfulrefreshMatViewstick no longer unconditionally flipsmatViewsReady. It now callsmatViewShapesReadyfirst, sinceREFRESHsucceeds on an old-shape view andrefreshMatViewsalso returnsnilwhen it skipped due to the lock or activity gate.pg_catalog.pg_table_is_visiblefilters to the existence and column-listing queries inmatViewNeedsRebuildto avoid matching same-named views in other schemas.(bool, error)signature.Type of change
Affected areas
How to test
go test ./framework/logstore/...The existing
TestEnsureMatViewsSharesRefreshAdvisoryLocktest now asserts theboolreturn value in both the lock-held and lock-free cases. The scale migration test and perf tests have been updated to match the new signature.To validate the rolling-deploy scenario specifically: run two instances sharing the same Postgres database against a schema that is missing the new matview columns, confirm that the replica that loses the lock race does not enable the matview read path until
matViewShapesReadyreturns true.Breaking changes
Internal function signature change only (
ensureMatViewsreturn type). No exported API is affected.Security considerations
pqTextArrayescapes backslashes and double-quotes. Column names passed to it come from package-level constants, not user input, but the escaping is correct regardless.Checklist