sql: elide recursion in the row iterator #131155
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
mw5h
force-pushed
the
elide_iter_recursion
branch
from
cbe3c05
to
c0364b3
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mw5h)
pkg/sql/internal.go line 537 at r1 (raw file):
func (r *rowsIterator) Next(ctx context.Context) (bool, error) { for !r.done && r.lastErr == nil {
super nit: r.done is never set inside the loop. An explicit if r.done { return false r.lastErr } block before the loop might be easier to read.
pkg/sql/internal.go line 549 at r1 (raw file):
if err != nil || done { r.lastErr = err break
super nit: An explicit break is used here to exit the loop, but in cases below where r.lastErr is set the for condition is used to break the loop. I find the inconsistency a bit confusing. But I don't feel too strongly. I suppose that would alter the semantics a bit, since before a call to Next after iteration is done would re-invoke r.Close()... I'm hoping that was unintentional though, since it seems wrong.
pkg/sql/internal.go line 573 at r1 (raw file):
if data.cols != nil { r.lastErr = errors.AssertionFailedf("unexpectedly receieved non-nil cols in Next: %v", data)
nit: receieved => received
pkg/sql/internal.go line 582 at r1 (raw file):
r.done = true _ = r.Close()
nit: the previous comment for ignoring this error was helpful, so maybe add it back?
// We can ignore the returned error because Close() will update
// r.lastErr if necessary.
pkg/sql/internal.go line 585 at r1 (raw file):
if r.errCallback != nil { r.lastErr = r.errCallback(r.lastErr) r.errCallback = nil
The previous semantics are a bit mind-bending, but I believe the logic around errCallback here is the same as before - errCallback is always called for the last iteration, regardless of the presence of an error or not, and then nilled so it is never called again. ✔️
That being said, it doesn't exactly match the comment for errCallback:
// errCallback is an optional callback that will be called exactly once
// before an error is returned by Next() or Close().
It might be good to update the comment to make it explicit that errCallback is always called once iteration is complete, even if the err is nil. Or perhaps better, change this to only call the callback if the err is not nil.
pkg/sql/internal.go line 585 at r1 (raw file):
r.rowsAffected++ // No need to make a copy because streamingCommandResult does that // for us.
nit: leave this comment about the copy
There was a problem hiding this comment.
Reviewable status:
pkg/sql/internal.go line 537 at r1 (raw file):
Meant to add this here:
I suppose that would alter the semantics a bit, since before a call to
Nextafter iteration is done would re-invoker.Close()... I'm hoping that was unintentional though, since it seems wrong.
pkg/sql/internal.go line 549 at r1 (raw file):
I suppose that would alter the semantics a bit, since before a call to
Nextafter iteration is done would re-invoker.Close()... I'm hoping that was unintentional though, since it seems wrong.
Ignore this I mangled up my comments. It belonged in another one.
mw5h
force-pushed
the
elide_iter_recursion
branch
from
c0364b3
to
94720cb
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/internal.go line 537 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Meant to add this here:
I suppose that would alter the semantics a bit, since before a call to
Nextafter iteration is done would re-invoker.Close()... I'm hoping that was unintentional though, since it seems wrong.
I like this comment and implemented it, but it doesn't work because the iterator initialization code just sets r.done and doesn't actually close the iterator (or return an error). It's pretty confusing and I may refactor that next, but it's more complicated so I'd like to keep that separate. So, we do need to hit that Close(). In my brain, r.done == true should mean that we've already closed.
pkg/sql/internal.go line 585 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
The previous semantics are a bit mind-bending, but I believe the logic around
errCallbackhere is the same as before -errCallbackis always called for the last iteration, regardless of the presence of an error or not, and then nilled so it is never called again. ✔️That being said, it doesn't exactly match the comment for
errCallback:// errCallback is an optional callback that will be called exactly once // before an error is returned by Next() or Close().It might be good to update the comment to make it explicit that
errCallbackis always called once iteration is complete, even if the err is nil. Or perhaps better, change this to only call the callback if the err is not nil.
I'm going with "let's make the comment correct" and we'll see what the tests tell us.
Previously, the row iterator would self-recurse to process partial results and would sometimes blow up the stack as a result. Attempts were made to limit the amount of recursion, but would still sometimes explode the stack. In this patch, we rewrite the function using a retry loop. Along the way, we simplify the logic to make the function more readable. Fixes: cockroachdb#129963 Release note: None
mw5h
force-pushed
the
elide_iter_recursion
branch
from
94720cb
to
2552b57
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
pkg/sql/internal.go line 537 at r1 (raw file):
Previously, mw5h (Matt White) wrote…
I like this comment and implemented it, but it doesn't work because the iterator initialization code just sets r.done and doesn't actually close the iterator (or return an error). It's pretty confusing and I may refactor that next, but it's more complicated so I'd like to keep that separate. So, we do need to hit that Close(). In my brain, r.done == true should mean that we've already closed.
Ahh, I guess that happens here:
Line 1396 in 09906d2
Good catch, I missed that. SGTM to leave as-is for now.
pkg/sql/internal.go line 585 at r1 (raw file):
Previously, mw5h (Matt White) wrote…
I'm going with "let's make the comment correct" and we'll see what the tests tell us.
👍
|
bors r+ |
There was a problem hiding this comment.
Thanks for the nice cleanup! I also realized that this PR fixed a bug - #133734.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
Previously, the row iterator would self-recurse to process partial results and would sometimes blow up the stack as a result. Attempts were made to limit the amount of recursion, but would still sometimes explode the stack. In this patch, we rewrite the function using a retry loop. Along the way, we simplify the logic to make the function more readable.
Fixes: #129963
Release note: None