Drop the never-written warning/critical_behind_threshold columns (#5173) - #5184
Merged
Conversation
`warning_behind_threshold` and `critical_behind_threshold` were created in DDL, listed in every extended-tracking SELECT, and hydrated into `ShardState` — and written by nothing, in any repo. Every read of them returned NULL, always. Nothing ever owned the value. In JasperFx the two `ShardState` properties are declared and never assigned or read; in CritterWatch the only threshold code path is a stub that logs and discards, and every lag threshold lives console-side. So this drops them rather than wiring them: two columns of storage, two entries in every extended-tracking SELECT and two selector ordinals, all carrying nothing. Existing deployments get an `alter table ... drop column` on their next `ApplyAllConfiguredChangesToDatabaseAsync()`, which is lossless because the columns were provably always NULL. The `failure_*` columns deliberately trail the extended block so `ShardStateSelector`'s positional ordinals stay stable; removing two columns from the middle of that block shifts them, so the existing #5048 round-trip tests are the guard and stay green. The two `ShardState` properties remain in JasperFx (unassigned and unread there too) — a hand-off, not something Marten can remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CTtw2kVRSZKp1p5RTxgAy
jeremydmiller
added a commit
that referenced
this pull request
Aug 4, 2026
…chema (#5173) (#5188) #5184 removed warning_behind_threshold / critical_behind_threshold from three places: the DDL, the extended-tracking SELECT, and ShardStateSelector's ordinals. The reasoning holds -- nothing in any repo ever wrote or read them, and every lag threshold lives in the monitoring tool -- and the read-side half of it stands. Only the DDL is restored. The cost of removing them was never the two always-NULL columns, it is the schema CHANGE. Measured against PostgreSQL, on a store with event sourcing registered, an mt_event_progression carrying a column the configuration no longer declares gives: CreateMigrationAsync().Difference = Update (mt_event_progression=Update) AssertDatabaseMatchesConfigurationAsync() -> throws ApplyAllConfiguredChangesToDatabaseAsync() -> drops the column So an upgrade is not the no-op #5184's commit message implied. Two things happen to existing deployments. A host running AutoCreate.None with a schema assertion at boot -- the standard production posture -- does not migrate, it refuses to start until someone migrates it. And a host that does apply runs `alter table ... drop column`, which in PostgreSQL is O(1) metadata but needs ACCESS EXCLUSIVE on a small, hot table that every running daemon writes; while that lock waits it blocks every reader and writer behind it. Two unused columns are much cheaper than asking every deployment to absorb that, and keeping them also means a freshly created database and an upgraded one have the same shape rather than diverging on a detail nothing reads. If they are ever removed for real it has to be a documented migration step rather than a silent consequence of upgrading a package. The tests pin both halves and both fail with the columns taken back out. Note the `AddEventType` in each: without an event type registered the event store's tables are not part of the schema migration at all, every delta comes back None, and any assertion about this table passes vacuously -- which is how the original version of this test was green against both shapes. Claude-Session: https://claude.ai/code/session_016v2Aijyo8MX2AdPUZL5VtG Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 5, 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
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.
Closes #5173.
The state of things
warning_behind_thresholdandcritical_behind_thresholdhad a complete read path and no write path:EventProgressionTablewhenEnableExtendedProgressionTrackingis on;ProjectionProgressStatement'sextendedColumns;ShardState.WarningBehindThreshold/.CriticalBehindThresholdbyShardStateSelector.And written by nothing, in any repo. They are absent from
WriteExtendedProgressionAsync'sUPDATE … FROM unnest(…)and from the legacymt_mark_event_progression_extended.sql, and nothing else writes extended columns. Every read returned NULL, always.Direction: drop, not wire
Nothing ever owned the value. In JasperFx both
ShardStateproperties are declared and never assigned or read — a repo-wide grep returns only the two declarations, no tests, no callers. In CritterWatch the only threshold code path isPushAgentThresholdsHandler, a stub that logs and discards (its computedagentUriis never used); every lag threshold lives console-side inProjectionAlertDefaults/AlertThresholdResolver.Wiring them would mean designing who owns the value, how it is pushed and what reads it — a real design question with no current demand. Dropping them removes two columns of storage, two entries in every extended-tracking SELECT, and two selector ordinals that carry nothing.
Migration note
Existing deployments with extended tracking on get an
alter table {schema}.mt_event_progression drop column …on their nextApplyAllConfiguredChangesToDatabaseAsync(). That is lossless: the columns were provably always NULL, and no query anywhere selects them any more.Ordinals
The issue flags this correctly —
ShardStateSelectorwalks positionally, and thefailure_*columns are deliberately kept trailing (ProjectionProgressStatement.cs:45-46) so their ordinals stay stable. Removing two columns from the middle of the extended block shifts everything after them, so the existing #5048 round-trip tests are the real guard here. They stay green (12/12 acrossBug_5048+extended_progression_*+ the new test, net10.0).New test
Bug_5173_behind_threshold_columns_removedasserts the two columns are no longer created and that the eight that carry something still are.Hand-off
ShardState.WarningBehindThreshold(Projections/ShardState.cs:116) and.CriticalBehindThreshold(:122) remain in JasperFx, still unassigned and unread. Removing them is a JasperFx change, and harmless to leave in place meanwhile — nothing sets them now.🤖 Generated with Claude Code
https://claude.ai/code/session_017CTtw2kVRSZKp1p5RTxgAy