sql: remove unused dropOwnedByNode#137739
Conversation
|
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 |
be5f13a to
dd9588e
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner)
rafiss
left a comment
There was a problem hiding this comment.
i'm not sure if we should go with this PR. it would cause the following noisy behavior in explicit transactions:
root@localhost:26257/defaultdb> begin;
BEGIN
root@localhost:26257/defaultdb OPEN> drop owned by root;
ERROR: internal error: unknown opaque statement *tree.DropOwnedBy
SQLSTATE: XX000
DETAIL: stack trace:
pkg/sql/opaque.go:312: planOpaque()
pkg/sql/opaque.go:59: buildOpaque()
pkg/sql/opt/optbuilder/opaque.go:65: tryBuildOpaque()
pkg/sql/opt/optbuilder/builder.go:483: buildStmt()
pkg/sql/opt/optbuilder/builder.go:316: buildStmtAtRootWithScope()
pkg/sql/opt/optbuilder/builder.go:297: buildStmtAtRoot()
pkg/sql/opt/optbuilder/builder.go:276: Build()
pkg/sql/plan_opt.go:824: buildExecMemo()
pkg/sql/plan_opt.go:260: makeOptimizerPlan()
pkg/sql/conn_executor_exec.go:3187: makeExecPlan()
pkg/sql/conn_executor_exec.go:2729: dispatchToExecutionEngine()
pkg/sql/conn_executor_exec.go:1025: execStmtInOpenState()
pkg/sql/conn_executor_exec.go:164: func2()
pkg/sql/conn_executor_exec.go:4334: execWithProfiling()
pkg/sql/conn_executor_exec.go:163: execStmt()
pkg/sql/conn_executor.go:2381: func1()
pkg/sql/conn_executor.go:2386: execCmd()
pkg/sql/conn_executor.go:2303: run()
pkg/sql/conn_executor.go:999: ServeConn()
pkg/sql/pgwire/conn.go:252: processCommands()
pkg/sql/pgwire/server.go:1197: func4()
src/runtime/asm_arm64.s:1222: goexit()
a related issue is #137615; we want to move all these statements that are unimplemented in the legacy schema changer to one place.
`DROP OWNED BY` is supported only in the declarative schema changer and `dropOwnedByNode` is not used, so it has been removed. Release note: None
dd9588e to
abbbbc2
Compare
|
Ahh, good catch @rafiss. I changed the PR to just remove the plan node, not the unimplemented error code path. |
rafiss
left a comment
There was a problem hiding this comment.
thanks for the cleanup!
Reviewed 1 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:
|
TFTRs! bors r+ |
DROP OWNED BYis supported only in the declarative schema changer anddropOwnedByNodeis not used, so it has been removed.Epic: None
Release note: None