Skip to content
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

sql: be safer about swapping out IVarHelpers #22310

Merged
merged 1 commit into from Feb 2, 2018

Conversation

justinj
Copy link
Contributor

@justinj justinj commented Feb 1, 2018

Fixes #22249.

This fix is admittedly somewhat ad-hoc, though I argue that it is
strictly less ad-hoc than the system we had before! The provided logic
test would panic because there was a place in the code where we were not
appropriately resetting an IVarHelper after swapping it out, leaving an
EvalContext with a nil IVarHelper.

I think this swapping out of IVarHelpers is something that has evolved
organically and has never been a particularly principled thing for us to
do. We should at some point figure out what it means for us to evaluate
a new expression with different indexed var bindings with the same
"context".

Release note (bug fix): Fix a panic with certain queries involving
REGCLASS

@justinj justinj requested review from knz, solongordon and a team February 1, 2018 22:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Feb 1, 2018

Looks good!


Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/sql/join_predicate.go, line 321 at r1 (raw file):

		copy(p.curRow[:len(leftRow)], leftRow)
		copy(p.curRow[len(leftRow):], rightRow)
		oldIVarHelper := ctx.IVarHelper

curious: why no push/pop here?


Comments from Reviewable

@justinj
Copy link
Contributor Author

justinj commented Feb 1, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/sql/join_predicate.go, line 321 at r1 (raw file):

Previously, knz (kena) wrote…

curious: why no push/pop here?

Oversight! I grepped for IVarHelper = nil. Will fix.


Comments from Reviewable

@justinj
Copy link
Contributor Author

justinj commented Feb 1, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/sql/join_predicate.go, line 321 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Oversight! I grepped for IVarHelper = nil. Will fix.

Oh I see I added that line, it was still an oversight, this was me experimenting and I forgot to change it back.


Comments from Reviewable

@justinj justinj force-pushed the regclass22249 branch 2 times, most recently from 5448cd8 to 6427d02 Compare February 2, 2018 16:00
Fixes cockroachdb#22249.

This fix is admittedly somewhat ad-hoc, though I argue that it is
strictly less ad-hoc than the system we had before! The provided logic
test would panic because there was a place in the code where we were not
appropriately resetting an IVarHelper after swapping it out, leaving an
EvalContext with a nil IVarHelper.

I think this swapping out of IVarHelpers is something that has evolved
organically and has never been a particularly principled thing for us to
do. We should at some point figure out what it means for us to evaluate
a new expression with different indexed var bindings with the same
"context".

Release note (bug fix): Fix a panic with certain queries involving
REGCLASS
@knz
Copy link
Contributor

knz commented Feb 2, 2018

:lgtm:


Reviewed 2 of 2 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@justinj
Copy link
Contributor Author

justinj commented Feb 2, 2018

TFTR!

@justinj justinj merged commit 5e9f236 into cockroachdb:master Feb 2, 2018
@justinj justinj deleted the regclass22249 branch February 2, 2018 19:06
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.

sql: panic in join query
3 participants