Fix MIR's could_run_expensive_function's quadraticness#35470
Open
ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
Open
Fix MIR's could_run_expensive_function's quadraticness#35470ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
could_run_expensive_function's quadraticness#35470ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
Conversation
ggevay
added
T-performance
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
Contributor
|
Merging #35413 now |
ggevay
force-pushed
the
could_run_expensive_function-mir
branch
2 times, most recently
from
c375ae7 to
e03dc78
Compare
ggevay
added a commit
that referenced
this pull request
Mar 17, 2026
`auto_run_on_catalog_server` was not looking inside the EXPLAIN plan when it was a SUBSCRIBE, so it was just falling back to the default case. (Actual, i.e. non-explain SUBSCRIBEs are correct.) Motivation: Needed for testing #35470
ggevay
force-pushed
the
could_run_expensive_function-mir
branch
from
e03dc78 to
558c4a5
Compare
maheshwarip
pushed a commit
that referenced
this pull request
Mar 19, 2026
`auto_run_on_catalog_server` was not looking inside the EXPLAIN plan when it was a SUBSCRIBE, so it was just falling back to the default case. (Actual, i.e. non-explain SUBSCRIBEs are correct.) Motivation: Needed for testing #35470
frankmcsherry
approved these changes
Mar 20, 2026
Contributor
frankmcsherry
left a comment
frankmcsherry
left a comment
There was a problem hiding this comment.
The code looks good. I think it could benefit from a comment about what that hunk of code is trying to do, for future generations. Fine to do this async after it merges, but I can't actually tell what the stack of closures does without reading the code (and .. we shouldn't want to have to read the code, imo).
ggevay
force-pushed
the
could_run_expensive_function-mir
branch
from
558c4a5 to
c2b3849
Compare
Contributor
Author
|
The function does have a doc comment with a high-level overview (which I've now tweaked a bit). Or did you mean something like an intermediate-level comment, which would be more detailed than the doc comment but less detailed than the code? |
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
3 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.
MIR's
could_run_expensive_functionwas accidentally quadratic.try_visit_scalarswas visiting the entire tree, and thevisit_prewas running this for every node of the tree. This is a really bad performance bug, because this runs on the coordinator's main task, so any big SUBSCRIBE (in terms of plan size) would make the coordinator stuck for a while.(Follow-up to #35413, which was fixing a bug in HIR's
could_run_expensive_function; cc @def- )