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.

Sign up for GitHub

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

Jump to bottom

sql: panic in UPDATE with subquery (v2.0.6) #32054

Closed
tbg opened this issue Oct 31, 2018 · 6 comments · Fixed by #34804
Closed

sql: panic in UPDATE with subquery (v2.0.6) #32054

tbg opened this issue Oct 31, 2018 · 6 comments · Fixed by #34804
Labels
A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. C-investigation Further steps needed to qualify. C-label will change. O-sentry Originated from an in-the-wild panic report. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.

Comments

@tbg
Copy link
Member

tbg commented Oct 31, 2018 •
edited by jordanlewis
Loading

https://sentry.io/cockroach-labs/cockroachdb/issues/748121057/

conn_executor.go:521: panic while executing 1 statements: UPDATE _ SET (, ) = (SELECT ( + $1), $2 FROM _ AS _ INNER JOIN _ AS _ ON . = . WHERE (._ = $3) AND (. = )) WHERE ( = $3) AND ((_ + $1) >= _): caused by

github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func2

stacktrace: {u'frames': [{u'function': u'Next', u'abs_path': u'/go/src/github.com/cockroachdb/cockroach/pkg/sql/update.go', u'module': u'github.com/cockroachdb/cockroach/pkg/sql.(*updateNode)', u'filename': u'github.com/cockroachdb/cockroach/pkg/sql/update.go', u'lineno': 329, u'in_app': True}, {u'function': u'extractValues', u'abs_path': u'/go/src/github.com/cockroachdb/cockroach/pkg/sql/update.go', u'module': u'github.com/cockroachdb/cockroach/pkg/sql.(*tupleSlot)', u'filename': u'github.com/cockroachdb/cockroach/pkg/sql/update.go', u'lineno': 508, u'in_app': True}, {u'function': u'panicdottypeI', u'abs_path': u'/usr/local/go/src/runtime/iface.go', u'module': u'runtime', u'filename': u'runtime/iface.go', u'lineno': 254, u'in_app': False}, {u'function': u'panicdottypeE', u'abs_path': u'/usr/local/go/src/runtime/iface.go', u'module': u'runtime', u'filename': u'runtime/iface.go', u'lineno': 244, u'in_app': False}, {u'function': u'gopanic', u'abs_path': u'/usr/local/go/src/runtime/panic.go', u'module': u'runtime', u'filename': u'runtime/panic.go', u'lineno': 505, u'in_app': False}, {u'function': u'call32', u'abs_path': u'/usr/local/go/src/runtime/asm_amd64.s', u'module': u'runtime', u'filename': u'runtime/asm_amd64.s', u'lineno': 573, u'in_app': False}, {u'function': u'func2', u'abs_path': u'/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go', u'module': u'github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn', u'filename': u'github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go', u'lineno': 489, u'in_app': True}]}
type: *log.safeError
value: conn_executor.go:521: panic while executing 1 statements: UPDATE _ SET (, ) = (SELECT ( + $1), $2 FROM _ AS _ INNER JOIN _ AS _ ON . = . WHERE (._ = $3) AND (. = )) WHERE ( = $3) AND ((_ + $1) >= _): caused by 

github.com/cockroachdb/cockroach/pkg/sql/update.go in extractValues at line 508
github.com/cockroachdb/cockroach/pkg/sql/update.go in Next at line 329
@tbg tbg added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-sentry Originated from an in-the-wild panic report. labels Oct 31, 2018
@knz knz added the A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. label Oct 31, 2018
@knz
Copy link
Contributor

knz commented Oct 31, 2018

This is in v2.0.6

@knz knz added the S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting. label Oct 31, 2018
@knz knz changed the title sentry: conn_executor.go:521: panic while executing 1 statements: UPDATE _ SET (_, _) = (SELECT (_ + $1), $2 FROM _ AS _ INNER JOIN _ AS _ ON _._ = _._ WHERE (_._ = $3) AND (_._ = _)) WHERE (_ = $3) AND ((_ + $1) >= _): caused by <redacted> sql: panic in UPDATE with subquery (v2.0.6) Nov 21, 2018
@knz knz added C-investigation Further steps needed to qualify. C-label will change. and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Nov 21, 2018
@jordanlewis
Copy link
Member 

func (ts tupleSlot) extractValues(row tree.Datums) tree.Datums {
        return row[ts.sourceIndex].(*tree.DTuple).D
}

The panic is on the return line.

@knz
Copy link
Contributor

knz commented Feb 11, 2019

IIRC @BramGruneir fixed this in master/2.2; @awoods187 suggests to backport to 2.0.

@BramGruneir
Copy link
Member

BramGruneir commented Feb 11, 2019 via email

@knz
Copy link
Contributor

knz commented Feb 11, 2019

Ok so I went and double checked and we corrected a panic for UPSERT, not UPDATE.

I also double-checked and this code was not changed since 2.0, so if there's a bug there, it's still in master.

@knz
Copy link
Contributor

knz commented Feb 11, 2019

Hah! found the bug. Both in master and 2.0. PR incoming.

This was referenced Feb 11, 2019
Merged
Merged
Merged
Merged
craig bot pushed a commit that referenced this issue Feb 12, 2019
@knz
Merge #34804
b80d241 
34804: sql: prevent UPDATE from crashing if subquery returns no rows r=knz a=knz

Fixes #32054.

Note: only the heuristic planner was affected. The CBO was handling
this case fine already.

Release note (bug fix): When using `UPDATE SET (a,b)
= (...subquery...)`, CockroachDb will not crash any more if the
subquery returns no rows.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig craig bot closed this as completed in #34804 Feb 12, 2019
craig bot pushed a commit that referenced this issue Feb 13, 2019
@knz
Merge #34801
8ac64be 
34801: sql: fix the subquery limit optimization r=knz a=knz

Found this while investigating #32054.

A while ago the HP was equipped with an optimization: when a subquery
is planned for EXISTS or "max 1 row" (scalar context), a LIMIT is
applied on its data source. This ensures that the data source does not
fetch more rows than strictly necessary to determine the subquery
result:

- for EXISTS, only 0 or 1 row are needed to decide the boolean;
- for scalar contexts, only 0, 1 or 2 rows are needed to decide the
  outcome 0 or 2 yield an error, only 1 gets a valid result.

This optimization was temporarily broken for the scalar case when
`max1row` was introduced (when local exec was subsumed by distsql),
because the limit was remaining "on top" of `max1row` and not
propagated down. This patch places it "under" so it gets propagated
again.

Release note (performance improvement): subqueries used with EXISTS or
as a scalar value now avoid fetching more rows than needed to decide
the outcome.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-sql-mutations Mutation statements: UPDATE/INSERT/UPSERT/DELETE. C-investigation Further steps needed to qualify. C-label will change. O-sentry Originated from an in-the-wild panic report. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: prevent UPDATE from crashing if subquery returns no rows knz/cockroach
4 participants
@jordanlewis @knz @BramGruneir @tbg