Skip to content

Comments

persist: Poke at a fetch-time assert#35132

Open
bkirwi wants to merge 2 commits intoMaterializeInc:mainfrom
bkirwi:since-bug
Open

persist: Poke at a fetch-time assert#35132
bkirwi wants to merge 2 commits intoMaterializeInc:mainfrom
bkirwi:since-bug

Conversation

@bkirwi
Copy link
Contributor

@bkirwi bkirwi commented Feb 20, 2026

Motivation

https://github.com/MaterializeInc/database-issues/issues/10097

Description

This assert is failing at part fetch time:

assert!(
    PartialOrder::less_equal(inline_desc.since(), registered_desc.since()),
    "key={} inline={:?} registered={:?}",
    printable_name,
    inline_desc,
    registered_desc
);

It's hard to know exactly what went wrong here based on the evidence in the logs, but, two commits:

  • Generally speaking, these two descs should match exactly, because the data was all written together. The only case I can think of where that's not true is incremental compaction, where we patch in a new set of runs. This will normally advance the since, which is fine according to the above check. However, I think it is possible for it to regress the since if we somehow end up doing two incremental compaction runs in parallel, those two runs end up with different target sinces, and the one with the earlier since commits second. Kind of a weird series of events! But anyways now that invariant is enforced more explicitly.
  • Even in that case, any data in the batch will be earlier than the overall shard since, since we check that invariant aggressively and early. So any differences here won't actually be visible to any readers, because those readers should be reading at the overall shard since or later. Thus: I have turned the assert into a soft assert; it indicates a problem but not necessarily a user-facing consistency violation, and it's kinder not to brick a shard if we can avoid it.

This is surprising, but not necessarily fatal, and being unnecessarily
strict here renders a shard unreadable.
This is the only case I can think of offhand where we could end up with
a batch desc that doesn't precisely match its contents... user batches
are always zero and compaction batches are otherwise written atomically.
The circumstances where this would actually come up are pretty baroque,
but it seems possible, and anyways there's no cost to checking it.
@bkirwi bkirwi requested a review from a team as a code owner February 20, 2026 17:29
@github-actions
Copy link

Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone.

PR title guidelines

  • Use imperative mood: "Fix X" not "Fixed X" or "Fixes X"
  • Be specific: "Fix panic in catalog sync when controller restarts" not "Fix bug" or "Update catalog code"
  • Prefix with area if helpful: compute: , storage: , adapter: , sql:

Pre-merge checklist

  • The PR title is descriptive and will make sense in the git log.
  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).

@bkirwi bkirwi requested a review from DAlperin February 20, 2026 17:30
@bkirwi bkirwi changed the title [persist] Poke at a fetch-time persist assert persist: Poke at a fetch-time assert Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant