adapter: don't invalidate expr cache when indexes are added #34863
Conversation
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
This commit removes the `invalidate_for_index` logic that was responsible for invalidating all expr cache entries for objects that could potentially make use of a newly added index. There are two reasons to think we don't want this logic: * Currently, bootstrapping plans dataflows in ID order. A new index will always have a greater ID than existing objects, so will never be available during the planning of those objects, regardless of whether or not the cache entries for them are invalidated. * Even if the replanning worked as intended, we don't actually want it. Changing the plan means breaking compute reconciliation, which means causing unavailability until the affected dataflows have rehydrated. We do want replanning across version upgrades, but in that case the entire expr cache is cleared anyway, so no special handling is needed.
teskje
force-pushed
the
remove-invalidate_for_index
branch
from
e9ae259 to
f80a5ee
Compare
ggevay
approved these changes
Jan 30, 2026
| new_local_expressions, | ||
| new_global_expressions, | ||
| invalidate_ids, | ||
| Default::default(), |
Contributor
There was a problem hiding this comment.
All calls to update are now passing an empty set to the invalidate_ids parameter, so you could consider removing that parameter, and downstream things. (But it's also ok to leave that to follow-up PRs.)
Contributor
Author
There was a problem hiding this comment.
I left it in because I want to follow up with a PR that also updates the cache in response to DDL. And I suspect there is a good chance we'll need to pass this argument when removing plans on DROP.
Contributor
Author
|
TFTR! |
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.
This PR removes the
invalidate_for_indexlogic that was responsible for invalidating all expr cache entries for objects that could potentially make use of a newly added index.There are two reasons to think we don't want this logic:
Because of the first point, this is not expected to change anything about the current behavior. Because of the second point, this is a necessary (though not sufficient) precondition for merging #34859.
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.