From 442dc4389fedd9547f4c8d934c13ad77da572c9f Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 4 Mar 2021 09:34:17 -0800 Subject: [PATCH 1/2] 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 #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. --- pkg/sql/delete.go | 5 +- pkg/sql/insert.go | 5 +- .../testdata/logic_test/partial_index | 46 +++++++++++++++++++ pkg/sql/update.go | 4 +- pkg/sql/upsert.go | 14 +++--- 5 files changed, 61 insertions(+), 13 deletions(-) diff --git a/pkg/sql/delete.go b/pkg/sql/delete.go index e51d96794d83..b09ea39ad89d 100644 --- a/pkg/sql/delete.go +++ b/pkg/sql/delete.go @@ -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 { diff --git a/pkg/sql/insert.go b/pkg/sql/insert.go index 8fd546b5e055..7fa7b5a3e096 100644 --- a/pkg/sql/insert.go +++ b/pkg/sql/insert.go @@ -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 { diff --git a/pkg/sql/logictest/testdata/logic_test/partial_index b/pkg/sql/logictest/testdata/logic_test/partial_index index 1d2e7c06f0d5..0f72d2d63d7b 100644 --- a/pkg/sql/logictest/testdata/logic_test/partial_index +++ b/pkg/sql/logictest/testdata/logic_test/partial_index @@ -1749,3 +1749,49 @@ CREATE TABLE t58390 ( statement ok ALTER TABLE t58390 ALTER PRIMARY KEY USING COLUMNS (b, a) + +# Regression tests for #61414. Upsert execution should not error if partial +# index PUT and DEL columns are not the last columns in the input of the +# mutation. + +statement ok +create table t61414_a ( + k INT PRIMARY KEY +) + +statement ok +create table t61414_b ( + k INT PRIMARY KEY, + a STRING, + b INT REFERENCES t61414_a(k), + UNIQUE INDEX (a, b), + INDEX (a) WHERE a = 'x' +) + +statement ok +INSERT INTO t61414_a VALUES (2) + +statement ok +INSERT INTO t61414_b (k, a, b) +VALUES (1, 'a', 2) +ON CONFLICT (a, b) DO UPDATE SET a = excluded.a +WHERE t61414_b.a = 'x' +RETURNING k + +statement ok +SET experimental_enable_unique_without_index_constraints = true + +statement ok +CREATE TABLE t61414_c ( + k INT PRIMARY KEY, + a INT, + b INT, + c INT, + d INT, + INDEX (b) WHERE b > 0, + UNIQUE WITHOUT INDEX (b) WHERE b > 0, + FAMILY (k, a, c) +) + +statement ok +UPSERT INTO t61414_c (k, a, b, d) VALUES (1, 2, 3, 4) diff --git a/pkg/sql/update.go b/pkg/sql/update.go index 48ea5cf4a3ae..262e78a90520 100644 --- a/pkg/sql/update.go +++ b/pkg/sql/update.go @@ -297,8 +297,8 @@ func (u *updateNode) processSourceRow(params runParams, sourceVals tree.Datums) // from. var pm row.PartialIndexUpdateHelper if n := len(u.run.tu.tableDesc().PartialIndexes()); n > 0 { - partialIndexValOffset := len(u.run.tu.ru.FetchCols) + len(u.run.tu.ru.UpdateCols) + u.run.checkOrds.Len() + u.run.numPassthrough - partialIndexVals := sourceVals[partialIndexValOffset:] + offset := len(u.run.tu.ru.FetchCols) + len(u.run.tu.ru.UpdateCols) + u.run.checkOrds.Len() + u.run.numPassthrough + partialIndexVals := sourceVals[offset:] partialIndexPutVals := partialIndexVals[:n] partialIndexDelVals := partialIndexVals[n : n*2] diff --git a/pkg/sql/upsert.go b/pkg/sql/upsert.go index 7ebef9e6b3f3..8cafaf2212cf 100644 --- a/pkg/sql/upsert.go +++ b/pkg/sql/upsert.go @@ -143,14 +143,14 @@ func (n *upsertNode) processSourceRow(params runParams, rowVals tree.Datums) err // Create a set of partial index IDs to not add or remove entries from. var pm row.PartialIndexUpdateHelper - if len(n.run.tw.tableDesc().PartialIndexes()) > 0 { - partialIndexValOffset := len(n.run.insertCols) + len(n.run.tw.fetchCols) + len(n.run.tw.updateCols) + n.run.checkOrds.Len() + if numPartialIndexes := len(n.run.tw.tableDesc().PartialIndexes()); numPartialIndexes > 0 { + offset := len(n.run.insertCols) + len(n.run.tw.fetchCols) + len(n.run.tw.updateCols) + n.run.checkOrds.Len() if n.run.tw.canaryOrdinal != -1 { - partialIndexValOffset++ + offset++ } - partialIndexVals := rowVals[partialIndexValOffset:] - partialIndexPutVals := partialIndexVals[:len(partialIndexVals)/2] - partialIndexDelVals := partialIndexVals[len(partialIndexVals)/2:] + partialIndexVals := rowVals[offset:] + partialIndexPutVals := partialIndexVals[:numPartialIndexes] + partialIndexDelVals := partialIndexVals[numPartialIndexes : numPartialIndexes*2] err := pm.Init(partialIndexPutVals, partialIndexDelVals, n.run.tw.tableDesc()) if err != nil { @@ -159,7 +159,7 @@ func (n *upsertNode) processSourceRow(params runParams, rowVals tree.Datums) err // Truncate rowVals so that it no longer includes partial index predicate // values. - rowVals = rowVals[:partialIndexValOffset] + rowVals = rowVals[:offset] } // Verify the CHECK constraints by inspecting boolean columns from the input that From be44df720b76a686a4e514e98aa2c5a36a6c36e9 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 4 Mar 2021 09:40:00 -0800 Subject: [PATCH 2/2] ccl: add tests for implicitly partitioned partial unqiue indexes Release justification: This is a test-only change. Release note: None --- .../testdata/logic_test/partitioning_implicit | 107 ++++++++++++++---- .../testdata/logic_test/regional_by_row | 89 +++++++++++++-- 2 files changed, 161 insertions(+), 35 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit index bb54d159f253..a0af6848aa8c 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit +++ b/pkg/ccl/logictestccl/testdata/logic_test/partitioning_implicit @@ -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) ( @@ -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 @@ -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) @@ -639,7 +643,7 @@ vectorized: true │ │ label: buffer 1 │ │ │ └── • values -│ size: 7 columns, 1 row +│ size: 8 columns, 1 row │ ├── • constraint-check │ │ @@ -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) @@ -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 diff --git a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row index eff870ad39f2..2198939f0d43 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row +++ b/pkg/ccl/logictestccl/testdata/logic_test/regional_by_row @@ -517,10 +517,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) @@ -555,6 +551,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 @@ -566,6 +563,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 @@ -703,7 +701,7 @@ vectorized: true │ │ label: buffer 1 │ │ │ └── • values -│ size: 7 columns, 1 row +│ size: 8 columns, 1 row │ ├── • constraint-check │ │ @@ -711,7 +709,7 @@ vectorized: true │ │ │ └── • 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 │ │ @@ -730,7 +728,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) │ │ @@ -743,13 +741,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) │ @@ -808,7 +828,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) │ │ @@ -820,13 +840,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) │ @@ -874,7 +914,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) │ │ @@ -886,13 +926,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) │ @@ -924,6 +984,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 @@ -944,6 +1005,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) @@ -967,6 +1029,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 @@ -1003,6 +1067,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)