-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release-25.1: sql: don't disable streamer due to scanBufferNode #143167
release-25.1: sql: don't disable streamer due to scanBufferNode #143167
Conversation
0c43ba9
to
c1fb3d5
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
c1fb3d5
to
9de7529
Compare
In ed3f640 we disabled the streamer whenever the plan contains LocalPlanNode processor (which is just a wrapper around a `planNode`). This was done to prevent misuse of the txn API, and we knew at the time that we were disabling the streamer in more cases than strictly necessary. We just saw a support case where it was disabled due to `scanBufferNode`, so this commit includes `scanBufferNode` and `bufferNode` into the allow-list of planNodes that don't disable the streamer (since these don't interact with internal executors or the txn in any way). Also add a few trace messages around this code. Release note: None
9de7529
to
0dfc7b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)
Backport 1/1 commits from #143151 on behalf of @yuzefovich.
/cc @cockroachdb/release
In ed3f640 we disabled the streamer
whenever the plan contains LocalPlanNode processor (which is just
a wrapper around a
planNode
). This was done to prevent misuse of thetxn API, and we knew at the time that we were disabling the streamer in
more cases than strictly necessary. We just saw a support case where it
was disabled due to
scanBufferNode
, so this commit includesscanBufferNode
andbufferNode
into the allow-list of planNodes thatdon't disable the streamer (since these don't interact with internal
executors or the txn in any way).
Also add a few trace messages around this code.
Epic: None
Release note: None
Release justification: low-risk improvement.