Skip to content

Commit

Permalink
opt: Fix panic on insert with check constraint and dropping column
Browse files Browse the repository at this point in the history 
The panic occurs because the delete-only column is never referenced nor used by
the Insert statement, and yet the disambiguateColumns method was trying to get
its ColumnID. The fix is to skip over the column, since there's no need to
disambiguate a column that's never referenced.

Fixes cockroachdb#35611

Release note: None
  • Loading branch information
@andy-kimball
andy-kimball committed Mar 12, 2019
1 parent 0c6efb2 commit ddcab18
Show file tree
Hide file tree
Showing 5 changed files with 351 additions and 252 deletions.
14 changes: 14 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/insert
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,3 +672,17 @@ INSERT INTO xy (x, y) SELECT a, b FROM ab ORDER BY -b LIMIT 10 RETURNING *;

statement ok
DROP TABLE xy; DROP TABLE ab

subtest regression_35611

statement ok
CREATE TABLE t35611(a INT PRIMARY KEY, CHECK (a > 0))

statement ok
BEGIN; ALTER TABLE t35611 ADD COLUMN b INT

statement ok
INSERT INTO t35611 (a) VALUES (1)

statement ok
COMMIT
28 changes: 21 additions & 7 deletions pkg/sql/opt/optbuilder/mutation_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,12 @@ func (mb *mutationBuilder) addCheckConstraintCols() {
func (mb *mutationBuilder) disambiguateColumns() {
for i, n := 0, mb.tab.DeletableColumnCount(); i < n; i++ {
colName := mb.tab.Column(i).ColName()
colID := mb.mapToReturnColID(i)
colID := mb.mapToInputColID(i)
if colID == 0 {
// Column not involved in the statement, so skip it (e.g. a delete-only
// column in an Insert statement).
continue
}
for i := range mb.outScope.cols {
col := &mb.outScope.cols[i]
if col.name == colName {
Expand Down Expand Up @@ -441,16 +446,20 @@ func (mb *mutationBuilder) makeMutationPrivate(needResults bool) *memo.MutationP
// can be non-zero.
private.ReturnCols = make(opt.ColList, mb.tab.DeletableColumnCount())
for i, n := 0, mb.tab.ColumnCount(); i < n; i++ {
private.ReturnCols[i] = mb.mapToReturnColID(i)
private.ReturnCols[i] = mb.mapToInputColID(i)
if private.ReturnCols[i] == 0 {
panic(fmt.Sprintf("column %d is not available in the mutation input", i))
}
}
}

return private
}

// mapToReturnColID returns the ID of the input column that will provide the
// value for the corresponding return column. Columns take priority in this
// order:
// mapToInputColID returns the ID of the input column that provides the final
// value for the column at the given ordinal position in the table. This value
// might mutate the column, or it might be returned by the mutation statement,
// or it might not be used at all. Columns take priority in this order:
//
// upsert, update, fetch, insert
//
Expand All @@ -459,7 +468,11 @@ func (mb *mutationBuilder) makeMutationPrivate(needResults bool) *memo.MutationP
// available, then it overrides any fetch value. Finally, the relative priority
// of fetch and insert columns doesn't matter, since they're only used together
// in the upsert case where an upsert column would be available.
func (mb *mutationBuilder) mapToReturnColID(ord int) opt.ColumnID {
//
// If the column is never referenced by the statement, then mapToInputColID
// returns 0. This would be the case for delete-only columns in an Insert
// statement, because they're neither fetched nor mutated.
func (mb *mutationBuilder) mapToInputColID(ord int) opt.ColumnID {
switch {
case mb.upsertColList != nil && mb.upsertColList[ord] != 0:
return mb.upsertColList[ord]
Expand All @@ -474,7 +487,8 @@ func (mb *mutationBuilder) mapToReturnColID(ord int) opt.ColumnID {
return mb.insertColList[ord]

default:
panic("could not find return column")
// Column is never referenced by the statement.
return 0
}
}

Expand Down
66 changes: 41 additions & 25 deletions pkg/sql/opt/optbuilder/testdata/insert
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ CREATE TABLE mutation (
n INT,
"o:write-only" INT DEFAULT(10),
"p:write-only" INT AS (o + n) STORED,
"q:delete-only" INT
"q:delete-only" INT AS (m * p) STORED,
CHECK (m > 0)
)
----
TABLE mutation
Expand All @@ -59,8 +60,9 @@ TABLE mutation
├── o int (mutation)
├── p int (mutation)
├── q int (mutation)
└── INDEX primary
└── m int not null
├── INDEX primary
│ └── m int not null
└── CHECK (m > 0)

exec-ddl
CREATE TABLE checks (
Expand Down Expand Up @@ -1186,21 +1188,28 @@ insert mutation
│ ├── column2:7 => n:2
│ ├── column8:8 => o:3
│ └── column9:9 => p:4
├── check columns: check1:10(bool)
└── project
├── columns: column9:9(int) column1:6(int) column2:7(int) column8:8(int!null)
├── columns: check1:10(bool) column1:6(int) column2:7(int) column8:8(int!null) column9:9(int)
├── project
│ ├── columns: column8:8(int!null) column1:6(int) column2:7(int)
│ ├── values
│ │ ├── columns: column1:6(int) column2:7(int)
│ │ └── tuple [type=tuple{int, int}]
│ │ ├── const: 1 [type=int]
│ │ └── const: 2 [type=int]
│ ├── columns: column9:9(int) column1:6(int) column2:7(int) column8:8(int!null)
│ ├── project
│ │ ├── columns: column8:8(int!null) column1:6(int) column2:7(int)
│ │ ├── values
│ │ │ ├── columns: column1:6(int) column2:7(int)
│ │ │ └── tuple [type=tuple{int, int}]
│ │ │ ├── const: 1 [type=int]
│ │ │ └── const: 2 [type=int]
│ │ └── projections
│ │ └── const: 10 [type=int]
│ └── projections
│ └── const: 10 [type=int]
│ └── plus [type=int]
│ ├── variable: column8 [type=int]
│ └── variable: column2 [type=int]
└── projections
└── plus [type=int]
├── variable: column8 [type=int]
└── variable: column2 [type=int]
└── gt [type=bool]
├── variable: column1 [type=int]
└── const: 0 [type=int]

# Use RETURNING clause and ensure that mutation columns aren't projected.
build
Expand All @@ -1213,21 +1222,28 @@ insert mutation
│ ├── column2:7 => n:2
│ ├── column8:8 => o:3
│ └── column9:9 => p:4
├── check columns: check1:10(bool)
└── project
├── columns: column9:9(int) column1:6(int) column2:7(int) column8:8(int!null)
├── columns: check1:10(bool) column1:6(int) column2:7(int) column8:8(int!null) column9:9(int)
├── project
│ ├── columns: column8:8(int!null) column1:6(int) column2:7(int)
│ ├── values
│ │ ├── columns: column1:6(int) column2:7(int)
│ │ └── tuple [type=tuple{int, int}]
│ │ ├── const: 1 [type=int]
│ │ └── const: 2 [type=int]
│ ├── columns: column9:9(int) column1:6(int) column2:7(int) column8:8(int!null)
│ ├── project
│ │ ├── columns: column8:8(int!null) column1:6(int) column2:7(int)
│ │ ├── values
│ │ │ ├── columns: column1:6(int) column2:7(int)
│ │ │ └── tuple [type=tuple{int, int}]
│ │ │ ├── const: 1 [type=int]
│ │ │ └── const: 2 [type=int]
│ │ └── projections
│ │ └── const: 10 [type=int]
│ └── projections
│ └── const: 10 [type=int]
│ └── plus [type=int]
│ ├── variable: column8 [type=int]
│ └── variable: column2 [type=int]
└── projections
└── plus [type=int]
├── variable: column8 [type=int]
└── variable: column2 [type=int]
└── gt [type=bool]
├── variable: column1 [type=int]
└── const: 0 [type=int]

# Try to reference write-only mutation column in RETURNING clause.
build
Expand Down
144 changes: 88 additions & 56 deletions pkg/sql/opt/optbuilder/testdata/update
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,20 @@ CREATE TABLE mutation (
m INT PRIMARY KEY,
n INT,
"o:write-only" INT DEFAULT(10),
"p:write-only" INT AS (o + n) STORED
"p:write-only" INT AS (o + n) STORED,
"q:delete-only" INT AS (m * p) STORED,
CHECK (m > 0)
)
----
TABLE mutation
├── m int not null
├── n int
├── o int (mutation)
├── p int (mutation)
└── INDEX primary
└── m int not null
├── q int (mutation)
├── INDEX primary
│ └── m int not null
└── CHECK (m > 0)

exec-ddl
CREATE TABLE checks (
Expand Down Expand Up @@ -1222,77 +1226,98 @@ UPDATE mutation SET m=1
----
update mutation
├── columns: <none>
├── fetch columns: m:5(int) n:6(int) o:7(int) p:8(int)
├── fetch columns: m:6(int) n:7(int) o:8(int) p:9(int) q:10(int)
├── update-mapping:
│ ├── column9:9 => m:1
│ └── column10:10 => p:4
│ ├── column11:11 => m:1
│ └── column12:12 => p:4
├── check columns: check1:13(bool)
└── project
├── columns: column10:10(int) m:5(int!null) n:6(int) o:7(int) p:8(int) column9:9(int!null)
├── columns: check1:13(bool) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int) column11:11(int!null) column12:12(int)
├── project
│ ├── columns: column9:9(int!null) m:5(int!null) n:6(int) o:7(int) p:8(int)
│ ├── scan mutation
│ │ └── columns: m:5(int!null) n:6(int) o:7(int) p:8(int)
│ ├── columns: column12:12(int) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int) column11:11(int!null)
│ ├── project
│ │ ├── columns: column11:11(int!null) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ ├── scan mutation
│ │ │ └── columns: m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ └── projections
│ │ └── const: 1 [type=int]
│ └── projections
│ └── const: 1 [type=int]
│ └── plus [type=int]
│ ├── variable: o [type=int]
│ └── variable: n [type=int]
└── projections
└── plus [type=int]
├── variable: o [type=int]
└── variable: n [type=int]
└── gt [type=bool]
├── variable: column11 [type=int]
└── const: 0 [type=int]

# Test update that requires computed mutation column to be recalculated.
build
UPDATE mutation SET m=1, n=2
----
update mutation
├── columns: <none>
├── fetch columns: m:5(int) n:6(int) o:7(int) p:8(int)
├── fetch columns: m:6(int) n:7(int) o:8(int) p:9(int) q:10(int)
├── update-mapping:
│ ├── column9:9 => m:1
│ ├── column10:10 => n:2
│ └── column11:11 => p:4
│ ├── column11:11 => m:1
│ ├── column12:12 => n:2
│ └── column13:13 => p:4
├── check columns: check1:14(bool)
└── project
├── columns: column11:11(int) m:5(int!null) n:6(int) o:7(int) p:8(int) column9:9(int!null) column10:10(int!null)
├── columns: check1:14(bool) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int) column11:11(int!null) column12:12(int!null) column13:13(int)
├── project
│ ├── columns: column9:9(int!null) column10:10(int!null) m:5(int!null) n:6(int) o:7(int) p:8(int)
│ ├── scan mutation
│ │ └── columns: m:5(int!null) n:6(int) o:7(int) p:8(int)
│ ├── columns: column13:13(int) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int) column11:11(int!null) column12:12(int!null)
│ ├── project
│ │ ├── columns: column11:11(int!null) column12:12(int!null) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ ├── scan mutation
│ │ │ └── columns: m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ └── projections
│ │ ├── const: 1 [type=int]
│ │ └── const: 2 [type=int]
│ └── projections
│ ├── const: 1 [type=int]
│ └── const: 2 [type=int]
│ └── plus [type=int]
│ ├── variable: o [type=int]
│ └── variable: column12 [type=int]
└── projections
└── plus [type=int]
├── variable: o [type=int]
└── variable: column10 [type=int]
└── gt [type=bool]
├── variable: column11 [type=int]
└── const: 0 [type=int]

# Ensure that ORDER BY wildcard does not select mutation columns.
build
UPDATE mutation SET m=1 ORDER BY mutation.* LIMIT 10
----
update mutation
├── columns: <none>
├── fetch columns: m:5(int) n:6(int) o:7(int) p:8(int)
├── fetch columns: m:6(int) n:7(int) o:8(int) p:9(int) q:10(int)
├── update-mapping:
│ ├── column9:9 => m:1
│ └── column10:10 => p:4
│ ├── column11:11 => m:1
│ └── column12:12 => p:4
├── check columns: check1:13(bool)
└── project
├── columns: column10:10(int) m:5(int!null) n:6(int) o:7(int) p:8(int) column9:9(int!null)
├── columns: check1:13(bool) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int) column11:11(int!null) column12:12(int)
├── project
│ ├── columns: column9:9(int!null) m:5(int!null) n:6(int) o:7(int) p:8(int)
│ ├── limit
│ │ ├── columns: m:5(int!null) n:6(int) o:7(int) p:8(int)
│ │ ├── internal-ordering: +5,+6
│ │ ├── sort
│ │ │ ├── columns: m:5(int!null) n:6(int) o:7(int) p:8(int)
│ │ │ ├── ordering: +5,+6
│ │ │ └── scan mutation
│ │ │ └── columns: m:5(int!null) n:6(int) o:7(int) p:8(int)
│ │ └── const: 10 [type=int]
│ ├── columns: column12:12(int) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int) column11:11(int!null)
│ ├── project
│ │ ├── columns: column11:11(int!null) m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ ├── limit
│ │ │ ├── columns: m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ │ ├── internal-ordering: +6,+7
│ │ │ ├── sort
│ │ │ │ ├── columns: m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ │ │ ├── ordering: +6,+7
│ │ │ │ └── scan mutation
│ │ │ │ └── columns: m:6(int!null) n:7(int) o:8(int) p:9(int) q:10(int)
│ │ │ └── const: 10 [type=int]
│ │ └── projections
│ │ └── const: 1 [type=int]
│ └── projections
│ └── const: 1 [type=int]
│ └── plus [type=int]
│ ├── variable: o [type=int]
│ └── variable: n [type=int]
└── projections
└── plus [type=int]
├── variable: o [type=int]
└── variable: n [type=int]
└── gt [type=bool]
├── variable: column11 [type=int]
└── const: 0 [type=int]

# Use column "o" from higher scope that is shadowed by mutation column "o".
build
Expand All @@ -1308,22 +1333,29 @@ project
└── exists [type=bool]
└── update mutation
├── columns: mutation.m:7(int!null) mutation.n:8(int)
├── fetch columns: mutation.m:11(int) mutation.n:12(int) mutation.o:13(int) p:14(int)
├── fetch columns: mutation.m:12(int) mutation.n:13(int) mutation.o:14(int) p:15(int) q:16(int)
├── update-mapping:
│ ├── o:15 => mutation.m:7
│ └── column16:16 => p:10
│ ├── o:17 => mutation.m:7
│ └── column18:18 => p:10
├── check columns: check1:19(bool)
└── project
├── columns: column16:16(int) mutation.m:11(int!null) mutation.n:12(int) mutation.o:13(int) p:14(int) o:15(int)
├── columns: check1:19(bool) mutation.m:12(int!null) mutation.n:13(int) mutation.o:14(int) p:15(int) q:16(int) o:17(int) column18:18(int)
├── project
│ ├── columns: o:15(int) mutation.m:11(int!null) mutation.n:12(int) mutation.o:13(int) p:14(int)
│ ├── scan mutation
│ │ └── columns: mutation.m:11(int!null) mutation.n:12(int) mutation.o:13(int) p:14(int)
│ ├── columns: column18:18(int) mutation.m:12(int!null) mutation.n:13(int) mutation.o:14(int) p:15(int) q:16(int) o:17(int)
│ ├── project
│ │ ├── columns: o:17(int) mutation.m:12(int!null) mutation.n:13(int) mutation.o:14(int) p:15(int) q:16(int)
│ │ ├── scan mutation
│ │ │ └── columns: mutation.m:12(int!null) mutation.n:13(int) mutation.o:14(int) p:15(int) q:16(int)
│ │ └── projections
│ │ └── variable: mno.o [type=int]
│ └── projections
│ └── variable: mno.o [type=int]
│ └── plus [type=int]
│ ├── variable: mutation.o [type=int]
│ └── variable: mutation.n [type=int]
└── projections
└── plus [type=int]
├── variable: mutation.o [type=int]
└── variable: mutation.n [type=int]
└── gt [type=bool]
├── variable: o [type=int]
└── const: 0 [type=int]

# Try to return a mutation column.
build
Expand Down
Loading

0 comments on commit ddcab18

Please sign in to comment.