Skip to content

Commit

Permalink
Merge cockroachdb#61416
Browse files Browse the repository at this point in the history 
61416: sql: fix UPSERT with columns after partial index PUT and DEL columns r=mgartner a=mgartner

#### sql: fix UPSERT with columns after partial index PUT and DEL columns

This commit fixes a bug in UPSERT execution that was caused by the
incorrect assumption that there are never columns following the partial
index PUT and DEL columns in the mutation's input.

I was unable to reproduce this bug for INSERTs or DELETEs. However, I
updated the execution code for both out of caution. The logic for both
no longer makes the same assumption that partial index PUT and DEL
columns will be the last columns in the input.

Fixes cockroachdb#61414

Release justification: This is a low-risk fix for a bug causing UPSERT
statements to err when executed on tables with partial indexes.

Release note (bug fix): A bug which caused UPSERT and INSERT..ON
CONFLICT..DO UPDATE statements to fail on tables with both partial
indexes and foreign key references has been fixed. This bug has been
present since version 20.2.0.

#### ccl: add tests for implicitly partitioned partial unqiue indexes

Release justification: This is a test-only change.

Release note: None


Co-authored-by: Marcus Gartner <[email protected]>
  • Loading branch information
@mgartner
craig[bot] and mgartner committed Mar 4, 2021
2 parents 2581a86 + be44df7 commit 46dd319
Show file tree
Hide file tree
Showing 7 changed files with 222 additions and 48 deletions.
107 changes: 84 additions & 23 deletions pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,7 @@ CREATE TABLE public.t (
d int,
INDEX (a),
UNIQUE (b),
UNIQUE (c) WHERE d > 100,
INDEX (partition_by, c),
FAMILY (pk, pk2, partition_by, a, b, c, d)
) PARTITION ALL BY RANGE (partition_by) (
Expand Down Expand Up @@ -592,6 +593,8 @@ t_a_idx a false
t_a_idx partition_by true
t_b_key b false
t_b_key partition_by true
t_c_key c false
t_c_key partition_by true
t_partition_by_c_idx c false
t_partition_by_c_idx partition_by false

Expand All @@ -609,6 +612,7 @@ CREATE TABLE public.t (
CONSTRAINT "primary" PRIMARY KEY (pk ASC),
INDEX t_a_idx (a ASC),
UNIQUE INDEX t_b_key (b ASC),
UNIQUE INDEX t_c_key (c ASC) WHERE d > 100:::INT8,
INDEX t_partition_by_c_idx (partition_by ASC, c ASC),
INDEX created_idx (c ASC),
FAMILY fam_0_pk_pk2_partition_by_a_b_c_d (pk, pk2, partition_by, a, b, c, d)
Expand Down Expand Up @@ -639,7 +643,7 @@ vectorized: true
│ │ label: buffer 1
│ │
│ └── • values
│ size: 7 columns, 1 row
│ size: 8 columns, 1 row
│
├── • constraint-check
│ │
Expand All @@ -658,22 +662,51 @@ vectorized: true
│ └── • scan buffer
│ label: buffer 1
│
├── • constraint-check
│ │
│ └── • error if rows
│ │
│ └── • hash join (right semi)
│ │ equality: (b) = (column5)
│ │ right cols are key
│ │ pred: (column1 != pk) OR (column3 != partition_by)
│ │
│ ├── • scan
│ │ missing stats
│ │ table: t@t_b_key
│ │ spans: FULL SCAN
│ │
│ └── • scan buffer
│ label: buffer 1
│
└── • constraint-check
│
└── • error if rows
│
└── • hash join (right semi)
│ equality: (b) = (column5)
│ right cols are key
│ pred: (column1 != pk) OR (column3 != partition_by)
│
├── • scan
│ missing stats
│ table: t@t_b_key
│ spans: FULL SCAN
└── • limit
│ count: 1
│
└── • scan buffer
label: buffer 1
└── • lookup join
│ table: t@primary
│ equality: (partition_by, pk) = (partition_by,pk)
│ equality cols are key
│
└── • hash join
│ equality: (c) = (column6)
│ right cols are key
│ pred: (column1 != pk) OR (column3 != partition_by)
│
├── • scan
│ missing stats
│ table: t@t_c_key (partial index)
│ spans: FULL SCAN
│
└── • filter
│ estimated row count: 1
│ filter: column7 > 100
│
└── • scan buffer
label: buffer 1

statement ok
INSERT INTO t VALUES (1, 1, 1, 1, 1, 1, 1), (2, 2, 2, 2, 2, 2, 2)
Expand Down Expand Up @@ -715,22 +748,50 @@ vectorized: true
│ table: t@primary
│ spans: FULL SCAN
│
├── • constraint-check
│ │
│ └── • error if rows
│ │
│ └── • hash join (right semi)
│ │ equality: (b) = (column5)
│ │ right cols are key
│ │ pred: (upsert_pk != pk) OR (column3 != partition_by)
│ │
│ ├── • scan
│ │ missing stats
│ │ table: t@t_b_key
│ │ spans: FULL SCAN
│ │
│ └── • scan buffer
│ label: buffer 1
│
└── • constraint-check
│
└── • error if rows
│
└── • hash join (right semi)
│ equality: (b) = (column5)
│ right cols are key
│ pred: (upsert_pk != pk) OR (column3 != partition_by)
│
├── • scan
│ missing stats
│ table: t@t_b_key
│ spans: FULL SCAN
└── • limit
│ count: 1
│
└── • scan buffer
label: buffer 1
└── • lookup join
│ table: t@primary
│ equality: (partition_by, pk) = (partition_by,pk)
│ equality cols are key
│
└── • hash join
│ equality: (c) = (column6)
│ right cols are key
│ pred: (upsert_pk != pk) OR (column3 != partition_by)
│
├── • scan
│ missing stats
│ table: t@t_c_key (partial index)
│ spans: FULL SCAN
│
└── • filter
│ filter: column7 > 100
│
└── • scan buffer
label: buffer 1

# One row already exists, one row is new.
statement ok
Expand Down
89 changes: 77 additions & 12 deletions pkg/ccl/logictestccl/testdata/logic_test/regional_by_row
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,10 +531,6 @@ statement ok
DELETE FROM regional_by_row_table WHERE pk = 5;
CREATE UNIQUE INDEX uniq_idx ON regional_by_row_table(a) WHERE b > 0

statement ok
DELETE FROM regional_by_row_table WHERe pk = 6;
DROP INDEX uniq_idx

query TI
INSERT INTO regional_by_row_table (crdb_region, pk, pk2, a, b) VALUES
('ca-central-1', 7, 7, 8, 9)
Expand Down Expand Up @@ -569,6 +565,7 @@ ORDER BY pk
----
ap-southeast-2 1 1 2 3
ap-southeast-2 4 4 5 6
ca-central-1 6 6 5 -5
ca-central-1 7 7 8 9
ca-central-1 10 10 11 12
us-east-1 20 20 21 22
Expand All @@ -580,6 +577,7 @@ SELECT * FROM regional_by_row_table ORDER BY pk
pk pk2 a b j
1 1 2 3 {"a": "b"}
4 4 5 6 {"c": "d"}
6 6 5 -5 NULL
7 7 8 9 NULL
10 10 11 12 NULL
20 20 21 22 NULL
Expand Down Expand Up @@ -717,15 +715,15 @@ vectorized: true
│ │ label: buffer 1
│ │
│ └── • values
│ size: 7 columns, 1 row
│ size: 8 columns, 1 row
│
├── • constraint-check
│ │
│ └── • error if rows
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@primary
│ │ equality: (lookup_join_const_col_@22, column1) = (crdb_region,pk)
│ │ equality: (lookup_join_const_col_@23, column1) = (crdb_region,pk)
│ │ equality cols are key
│ │ pred: column15 != crdb_region
│ │
Expand All @@ -744,7 +742,7 @@ vectorized: true
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@regional_by_row_table_b_key
│ │ equality: (lookup_join_const_col_@37, column4) = (crdb_region,b)
│ │ equality: (lookup_join_const_col_@38, column4) = (crdb_region,b)
│ │ equality cols are key
│ │ pred: (column1 != pk) OR (column15 != crdb_region)
│ │
Expand All @@ -757,13 +755,35 @@ vectorized: true
│ └── • scan buffer
│ label: buffer 1
│
├── • constraint-check
│ │
│ └── • error if rows
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@uniq_idx (partial index)
│ │ equality: (lookup_join_const_col_@53, column3) = (crdb_region,a)
│ │ pred: (column1 != pk) OR (column15 != crdb_region)
│ │
│ └── • cross join
│ │ estimated row count: 3
│ │
│ ├── • values
│ │ size: 1 column, 3 rows
│ │
│ └── • filter
│ │ estimated row count: 1
│ │ filter: column4 > 0
│ │
│ └── • scan buffer
│ label: buffer 1
│
└── • constraint-check
│
└── • error if rows
│
└── • lookup join (semi)
│ table: regional_by_row_table@new_idx
│ equality: (lookup_join_const_col_@52, column3, column4) = (crdb_region,a,b)
│ equality: (lookup_join_const_col_@68, column3, column4) = (crdb_region,a,b)
│ equality cols are key
│ pred: (column1 != pk) OR (column15 != crdb_region)
│
Expand Down Expand Up @@ -822,7 +842,7 @@ vectorized: true
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@regional_by_row_table_b_key
│ │ equality: (lookup_join_const_col_@33, column5) = (crdb_region,b)
│ │ equality: (lookup_join_const_col_@35, column5) = (crdb_region,b)
│ │ equality cols are key
│ │ pred: (upsert_pk != pk) OR (column1 != crdb_region)
│ │
Expand All @@ -834,13 +854,33 @@ vectorized: true
│ └── • scan buffer
│ label: buffer 1
│
├── • constraint-check
│ │
│ └── • error if rows
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@uniq_idx (partial index)
│ │ equality: (lookup_join_const_col_@50, column4) = (crdb_region,a)
│ │ pred: (upsert_pk != pk) OR (column1 != crdb_region)
│ │
│ └── • cross join
│ │
│ ├── • values
│ │ size: 1 column, 3 rows
│ │
│ └── • filter
│ │ filter: column5 > 0
│ │
│ └── • scan buffer
│ label: buffer 1
│
└── • constraint-check
│
└── • error if rows
│
└── • lookup join (semi)
│ table: regional_by_row_table@new_idx
│ equality: (lookup_join_const_col_@48, column4, column5) = (crdb_region,a,b)
│ equality: (lookup_join_const_col_@65, column4, column5) = (crdb_region,a,b)
│ equality cols are key
│ pred: (upsert_pk != pk) OR (column1 != crdb_region)
│
Expand Down Expand Up @@ -888,7 +928,7 @@ vectorized: true
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@regional_by_row_table_b_key
│ │ equality: (lookup_join_const_col_@33, column5) = (crdb_region,b)
│ │ equality: (lookup_join_const_col_@35, column5) = (crdb_region,b)
│ │ equality cols are key
│ │ pred: (upsert_pk != pk) OR (column1 != crdb_region)
│ │
Expand All @@ -900,13 +940,33 @@ vectorized: true
│ └── • scan buffer
│ label: buffer 1
│
├── • constraint-check
│ │
│ └── • error if rows
│ │
│ └── • lookup join (semi)
│ │ table: regional_by_row_table@uniq_idx (partial index)
│ │ equality: (lookup_join_const_col_@50, column4) = (crdb_region,a)
│ │ pred: (upsert_pk != pk) OR (column1 != crdb_region)
│ │
│ └── • cross join
│ │
│ ├── • values
│ │ size: 1 column, 3 rows
│ │
│ └── • filter
│ │ filter: column5 > 0
│ │
│ └── • scan buffer
│ label: buffer 1
│
└── • constraint-check
│
└── • error if rows
│
└── • lookup join (semi)
│ table: regional_by_row_table@new_idx
│ equality: (lookup_join_const_col_@48, column4, column5) = (crdb_region,a,b)
│ equality: (lookup_join_const_col_@65, column4, column5) = (crdb_region,a,b)
│ equality cols are key
│ pred: (upsert_pk != pk) OR (column1 != crdb_region)
│
Expand Down Expand Up @@ -938,6 +998,7 @@ ORDER BY pk
crdb_region pk pk2 a b
ap-southeast-2 1 1 2 3
ap-southeast-2 4 4 5 6
ca-central-1 6 6 5 -5
ca-central-1 7 7 8 9
ca-central-1 10 10 11 12
us-east-1 20 20 21 22
Expand All @@ -958,6 +1019,7 @@ CREATE TABLE public.regional_by_row_table (
INDEX regional_by_row_table_a_idx (a ASC),
UNIQUE INDEX regional_by_row_table_b_key (b ASC),
INVERTED INDEX regional_by_row_table_j_idx (j),
UNIQUE INDEX uniq_idx (a ASC) WHERE b > 0:::INT8,
INDEX new_idx (a ASC, b ASC),
UNIQUE INDEX unique_b_a (b ASC, a ASC),
FAMILY fam_0_pk_pk2_a_b_j_crdb_region (pk, pk2, a, b, j, crdb_region)
Expand All @@ -981,6 +1043,8 @@ regional_by_row_table_b_key b false
regional_by_row_table_b_key crdb_region true
regional_by_row_table_j_idx crdb_region true
regional_by_row_table_j_idx j false
uniq_idx a false
uniq_idx crdb_region true
unique_b_a a false
unique_b_a b false
unique_b_a crdb_region true
Expand Down Expand Up @@ -1017,6 +1081,7 @@ CREATE TABLE public.regional_by_row_table (
INDEX regional_by_row_table_a_idx (a ASC),
UNIQUE INDEX regional_by_row_table_b_key (b ASC),
INVERTED INDEX regional_by_row_table_j_idx (j),
UNIQUE INDEX uniq_idx (a ASC) WHERE b > 0:::INT8,
INDEX new_idx (a ASC, b ASC),
UNIQUE INDEX unique_b_a (b ASC, a ASC),
FAMILY fam_0_pk_pk2_a_b_j_crdb_region (pk, pk2, a, b, j, crdb_region)
Expand Down
5 changes: 3 additions & 2 deletions pkg/sql/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,9 @@ func (d *deleteNode) processSourceRow(params runParams, sourceVals tree.Datums)
// satisfy the predicate and therefore do not exist in the partial index.
// This set is passed as a argument to tableDeleter.row below.
var pm row.PartialIndexUpdateHelper
if len(d.run.td.tableDesc().PartialIndexes()) > 0 {
partialIndexDelVals := sourceVals[d.run.partialIndexDelValsOffset:]
if n := len(d.run.td.tableDesc().PartialIndexes()); n > 0 {
offset := d.run.partialIndexDelValsOffset
partialIndexDelVals := sourceVals[offset : offset+n]

err := pm.Init(tree.Datums{}, partialIndexDelVals, d.run.td.tableDesc())
if err != nil {
Expand Down
5 changes: 3 additions & 2 deletions pkg/sql/insert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,9 @@ func (r *insertRun) processSourceRow(params runParams, rowVals tree.Datums) erro
// written to when they are partial indexes and the row does not satisfy the
// predicate. This set is passed as a parameter to tableInserter.row below.
var pm row.PartialIndexUpdateHelper
if len(r.ti.tableDesc().PartialIndexes()) > 0 {
partialIndexPutVals := rowVals[len(r.insertCols)+r.checkOrds.Len():]
if n := len(r.ti.tableDesc().PartialIndexes()); n > 0 {
offset := len(r.insertCols) + r.checkOrds.Len()
partialIndexPutVals := rowVals[offset : offset+n]

err := pm.Init(partialIndexPutVals, tree.Datums{}, r.ti.tableDesc())
if err != nil {
Expand Down
Loading

0 comments on commit 46dd319

Please sign in to comment.