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

opt: GROUP BY panics on invalid ARRAY aggregations #30412

Closed
rjnn opened this issue Sep 19, 2018 · 6 comments · Fixed by #30446
Closed

opt: GROUP BY panics on invalid ARRAY aggregations #30412

rjnn opened this issue Sep 19, 2018 · 6 comments · Fixed by #30446
Assignees
Labels
A-sql-optimizer SQL logical planning and optimizations. backport-2.1.x C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Milestone

Comments

@rjnn
Copy link
Contributor

rjnn commented Sep 19, 2018

Minimal reproduction:

select 0, unnest(array[0]) group by 1

panics with:

panic: top-level relational expression cannot have outer columns: (1) [recovered]
	panic: panic while executing 1 statements: SELECT _, unnest(ARRAY[_]) GROUP BY _; caused by top-level relational expression cannot have outer columns: (1)

This should gracefully fail, not crash the server.

@rjnn rjnn added S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting. backport-2.1.x labels Sep 19, 2018
@rjnn rjnn self-assigned this Sep 19, 2018
@knz knz added the A-sql-optimizer SQL logical planning and optimizations. label Sep 19, 2018
@knz
Copy link
Contributor

knz commented Sep 19, 2018

is this opt-specific?

@knz
Copy link
Contributor

knz commented Sep 19, 2018

(in general I'd advise copy-pasting the full stack trace in an issue report)

@knz knz added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Sep 19, 2018
@knz knz added this to the 2.1 milestone Sep 19, 2018
@rjnn
Copy link
Contributor Author

rjnn commented Sep 19, 2018

Yes, with the optimizer turned off, one gets the sane message

root@127.102.171.165:42635/defaultdb> select 0, unnest(array[0]) group by 1;                                                                                                           pq: column "unnest" must appear in the GROUP BY clause or be used in an aggregate function

Here's a full stack:

github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).closeWrapper(0xc001680a00, 0x309ed80, 0xc001408780, 0x25b24c0, 0xc00040e6f0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:675 +0x36f
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1(0xc001680a00, 0x309ed80, 0xc001408780)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:414 +0x61
panic(0x25b24c0, 0xc00040e6f0)
	/usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/cockroachdb/cockroach/pkg/sql/opt/xform.(*Optimizer).Optimize(0xc001681980, 0x0, 0x0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/opt/xform/optimizer.go:182 +0x222
github.com/cockroachdb/cockroach/pkg/sql.(*planner).makeOptimizerPlan(0xc001680e30, 0x309ee40, 0xc0013e6cf0, 0x30a2540, 0xc001410400, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:460 +0x960
github.com/cockroachdb/cockroach/pkg/sql.(*planner).optionallyUseOptimizer(0xc001680e30, 0x309ee40, 0xc0013e6cf0, 0xc000ff602a, 0x10, 0x29e7480, 0x9, 0x0, 0x1, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/planner.go:537 +0xc4
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine(0xc001680a00, 0x309ee40, 0xc0013e6cf0, 0x30a2540, 0xc001410400, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:766 +0x17d
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState(0xc001680a00, 0x309ee40, 0xc0013e6cf0, 0x30a2540, 0xc001410400, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:403 +0xaae
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt(0xc001680a00, 0x309ee40, 0xc0013e6cf0, 0x30a2540, 0xc001410400, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:95 +0x333
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run(0xc001680a00, 0x309ed80, 0xc001408780, 0xc00039d0b0, 0x0, 0x0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1079 +0x213a
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn(0xc000833800, 0x309ed80, 0xc001408780, 0x0, 0x0, 0xc000ff6040, 0x4, 0xc000ff602a, 0x10, 0x3080100, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:416 +0x1c8
github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl.func3(0xc000833800, 0x309ed80, 0xc001408780, 0xc0008d68c0, 0x5400, 0x15000, 0xc000786830, 0xc00039d0b0, 0xc00039d0a0, 0xc000357570, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:268 +0x148
created by github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:267 +0xedd

@rjnn
Copy link
Contributor Author

rjnn commented Sep 19, 2018

Local Execution catches this error case well into planning. I modified the code to emit the stack trace where the error was emitted:

goroutine 600 [running]:
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).closeWrapper(0xc00116ca00, 0x309ed80, 0xc0011c7f80, 0x28b8be0, 0xc001248fc0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:675 +0x36f
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1(0xc00116ca00, 0x309ed80, 0xc0011c7f80)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:414 +0x61
panic(0x28b8be0, 0xc001248fc0)
	/usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/cockroachdb/cockroach/pkg/sql.(*extractAggregatesVisitor).VisitPre(0xc001473540, 0x30a1e80, 0xc0016984e0, 0xc00037ac00, 0x0, 0x0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/group.go:737 +0x1452
github.com/cockroachdb/cockroach/pkg/sql/sem/tree.WalkExpr(0x307c280, 0xc001473540, 0x30a1e80, 0xc0016984e0, 0xc0016984e0, 0x16000, 0xc0016986c0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/sem/tree/walk.go:680 +0x56
github.com/cockroachdb/cockroach/pkg/sql.extractAggregatesVisitor.extract(0x309ee40, 0xc0016981e0, 0xc00116ce30, 0xc000483080, 0xc000fb89a0, 0xc000fb8c90, 0xc0016985a0, 0x0, 0x0, 0x30b3740, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/group.go:750 +0xec
github.com/cockroachdb/cockroach/pkg/sql.(*planner).groupBy(0xc00116ce30, 0x309ee40, 0xc0016981e0, 0xc000fb4cf0, 0xc000fb89a0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/group.go:280 +0x91e
github.com/cockroachdb/cockroach/pkg/sql.(*planner).SelectClause(0xc00116ce30, 0x309ee40, 0xc0016981e0, 0xc000fb4cf0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/render.go:284 +0x537
github.com/cockroachdb/cockroach/pkg/sql.(*planner).Select(0xc00116ce30, 0x309ee40, 0xc0016981e0, 0xc0011c7100, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/render.go:126 +0x57e
github.com/cockroachdb/cockroach/pkg/sql.(*planner).newPlan(0xc00116ce30, 0x309ee40, 0xc0016981e0, 0x30a2540, 0xc0011c7100, 0x0, 0x0, 0x0, 0xc001698300, 0x2, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:828 +0xd15
github.com/cockroachdb/cockroach/pkg/sql.(*planner).makePlan(0xc00116ce30, 0x309ee40, 0xc0016981e0, 0x30a2540, 0xc0011c7100, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/plan.go:304 +0x12a
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).dispatchToExecutionEngine(0xc00116ca00, 0x309ee40, 0xc0016981e0, 0x30a2540, 0xc0011c7100, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:771 +0xd7b
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmtInOpenState(0xc00116ca00, 0x309ee40, 0xc0016981e0, 0x30a2540, 0xc0011c7100, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:403 +0xaae
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).execStmt(0xc00116ca00, 0x309ee40, 0xc0016981e0, 0x30a2540, 0xc0011c7100, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:95 +0x333
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).run(0xc00116ca00, 0x309ed80, 0xc0011c7f80, 0xc000398640, 0x0, 0x0)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:1079 +0x213a
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn(0xc0007fb800, 0x309ed80, 0xc0011c7f80, 0x0, 0x0, 0xc001646009, 0x4, 0xc00164605c, 0x10, 0x30800c0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:416 +0x1c8
github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl.func3(0xc0007fb800, 0x309ed80, 0xc0011c7f80, 0xc0012d0700, 0x5400, 0x15000, 0xc00079f5b0, 0xc000398640, 0xc000398630, 0xc000353cc0, ...)
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:268 +0x148
created by github.com/cockroachdb/cockroach/pkg/sql/pgwire.(*conn).serveImpl
	/home/arjun/goworkspace/src/github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:267 +0xedd

This isn't caught at typechecking. Since it is only caught by the planning apparatus, opt will have to do a similar analysis and gracefully error out.

@rjnn rjnn assigned andy-kimball and unassigned rjnn Sep 19, 2018
@rjnn
Copy link
Contributor Author

rjnn commented Sep 19, 2018

Assigned to @andy-kimball for triage.

@andy-kimball
Copy link
Contributor

Rebecca, can you take a look? Looks like a probably SRF issue.

rytaft added a commit to rytaft/cockroach that referenced this issue Sep 20, 2018
Instead of panicking, we now throw an appropriate error.

Fixes cockroachdb#30412

Release note (bug fix): Fixed a panic that occurred when a
generator function such as unnest was used in the SELECT list
in the presence of GROUP BY.
craig bot pushed a commit that referenced this issue Sep 20, 2018
30405: roachtest: mark acceptance as stable r=petermattis a=tschottdorf

all of its subtests are already stable, but in running a test locally I
noticed that the top-level test was marked as passing as unstable. I'm
not sure, but this might mean that the top-level test would actually not
fail? Either way, better to mark it as stable explicitly.

We should also spend some thought on how diverging notions of Stable in
sub vs top level test are treated, not sure that this is well-defined.

Release note: None

30446: opt: fix panic when srf used with GROUP BY r=rytaft a=rytaft

Instead of panicking, we now throw an appropriate error.

Fixes #30412

Release note (bug fix): Fixed a panic that occurred when a
generator function such as unnest was used in the SELECT list
in the presence of GROUP BY.

30450: roachtest: remove now-unnecessary hack r=petermattis a=tschottdorf

Closes #27717.

Release note: None

30451: storage: give TestReplicateRemovedNodeDisruptiveElection more time r=petermattis a=tschottdorf

Perhaps:

Fixes #27253.

Release note: None

30452: storage: de-flake TestReplicaIDChangePending r=petermattis a=tschottdorf

setReplicaID refreshes the proposal and was thus synchronously writing
to the commandProposed chan. This channel could have filled up due to
an earlier reproposal already, deadlocking the test.

Fixes #28132.

Release note: None

30455: testcluster: improve AddReplicas check r=petermattis a=tschottdorf

AddReplicas was verifying that a replica had indeed been added, but
there's no guarantee that the replicate queue wouldn't have removed
it in the meantime. Attempt to work around this somewhat. The real
solution is not to provide that guarantee, but some tests likely
rely on it (and the failure is extremely rare, i.e. the new for
loop basically never runs).

Observed in #28368.

Release note: None

30456: storage: unskip TestClosedTimestampCanServe for non-race r=petermattis a=tschottdorf

Fixes #28607.

Release note: None

Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
@craig craig bot closed this as completed in #30446 Sep 20, 2018
rytaft added a commit to rytaft/cockroach that referenced this issue Sep 20, 2018
Instead of panicking, we now throw an appropriate error.

Fixes cockroachdb#30412

Release note (bug fix): Fixed a panic that occurred when a
generator function such as unnest was used in the SELECT list
in the presence of GROUP BY.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-optimizer SQL logical planning and optimizations. backport-2.1.x C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants