sql: support INSERT with partial UNIQUE WITHOUT INDEX constraints #60535
Conversation
|
This change is |
| func (h *uniqueCheckHelper) buildTableScan() (outScope *scope, ordinals []int) { | ||
| tabMeta := h.mb.b.addTable(h.mb.tab, tree.NewUnqualifiedTableName(h.mb.tab.Name())) | ||
| // TODO(mgartner): Should mutation columns be included here? Does uniqueness | ||
| // need to hold true for write-only columns? |
There was a problem hiding this comment.
I wasn't sure how to reason about write-only columns in this case. Any insight would be helpful!
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
2 times, most recently
from
790223c to
e94cdf2
Compare
There was a problem hiding this comment.
Nice work!
Reviewed 4 of 4 files at r1, 5 of 6 files at r2, 4 of 4 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)
pkg/sql/opt/exec/execbuilder/testdata/unique, line 79 at r3 (raw file):
a INT, b INT, UNIQUE WITHOUT INDEX (a) WHERE b > 0,
How about adding another partial unique without index constraint where the predicate contains the unique column?
pkg/sql/opt/optbuilder/mutation_builder.go, line 1324 at r1 (raw file):
name: inputCol.ColName(), typ: inputCol.DatumType(), }
Why can't you just use inputColMeta.Alias and inputColMeta.Type as the name/type? Are these differences in name/type relevant when there is an AS clause?
pkg/sql/opt/optbuilder/mutation_builder.go, line 1209 at r3 (raw file):
predStr, isPartial := uniqueConstraint.Predicate() if !isPartial { panic(errors.AssertionFailedf("unique constraints at ordinal %d is not a partial unique constraint", idx))
[nit] constraints -> constraint
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 285 at r2 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I wasn't sure how to reason about write-only columns in this case. Any insight would be helpful!
My guiding principle with this uniqueness stuff has been to try to make the unique constraints behave as close as possible to unique indexes. So I did a bit of testing, and it seems to me like we shouldn't need to include mutation columns here (yet). Here was my test:
[email protected]:26257/movr> create table t (k int primary key, v int unique);
CREATE TABLE
Time: 3ms total (execution 3ms / network 0ms)
[email protected]:26257/movr> begin;
BEGIN
Time: 0ms total (execution 0ms / network 0ms)
[email protected]:26257/movr OPEN> alter table t add column w int not null default 0;
ALTER TABLE
Time: 2ms total (execution 2ms / network 0ms)
[email protected]:26257/movr OPEN> create unique index w_idx on t (w);
CREATE INDEX
Time: 5ms total (execution 5ms / network 0ms)
[email protected]:26257/movr OPEN> insert into t values (3, 3), (4, 4);
INSERT 2
Time: 2ms total (execution 1ms / network 0ms)
[email protected]:26257/movr OPEN> commit;
ERROR: transaction committed but schema change aborted with error: (23505): failed to ingest index entries during backfill: duplicate key value violates unique constraint "w_idx"
SQLSTATE: XXA00
DETAIL: Key (w)=(0) already exists.
CONSTRAINT: w_idx
HINT: Some of the non-DDL statements may have committed successfully, but some of the DDL statement(s) failed.
Manual inspection may be required to determine the actual state of the database.
--
See: https://github.com/cockroachdb/cockroach/issues/42061
[email protected]:26257/movr> select * from t;
k | v
----+----
3 | 3
4 | 4
(2 rows)
Time: 1ms total (execution 1ms / network 0ms)
[email protected]:26257/movr> truncate t;
TRUNCATE
Time: 18ms total (execution 5ms / network 13ms)
[email protected]:26257/movr> SET experimental_enable_unique_without_index_constraints = true;
SET
Time: 0ms total (execution 0ms / network 0ms)
[email protected]:26257/movr> begin;
BEGIN
Time: 0ms total (execution 0ms / network 0ms)
[email protected]:26257/movr OPEN> alter table t add column w int not null default 0;
ALTER TABLE
Time: 2ms total (execution 2ms / network 0ms)
[email protected]:26257/movr OPEN> alter table t add constraint w_no_idx unique without index (w);
ALTER TABLE
Time: 5ms total (execution 5ms / network 0ms)
[email protected]:26257/movr OPEN> insert into t values (3, 3), (4, 4);
INSERT 2
Time: 1ms total (execution 1ms / network 0ms)
[email protected]:26257/movr OPEN> commit;
ERROR: transaction committed but schema change aborted with error: (23505): could not create unique constraint "w_no_idx"
SQLSTATE: XXA00
DETAIL: Key (w)=(0) is duplicated.
CONSTRAINT: w_no_idx
HINT: Some of the non-DDL statements may have committed successfully, but some of the DDL statement(s) failed.
Manual inspection may be required to determine the actual state of the database.
--
See: https://github.com/cockroachdb/cockroach/issues/42061
[email protected]:26257/movr> select * from t;
k | v
----+----
3 | 3
4 | 4
(2 rows)
Time: 1ms total (execution 1ms / network 0ms)
Notice that both the schema changes and insertions appear to succeed during the transactions both with and without indexes, and then the schema changes fail at commit time. This will obviously change once schema changes are transactional, and at that point maybe we will need to include mutation columns here.
Does that seem reasonable? If so, can you add a TODO?
(Note that this is different behavior from Postgres, since adding a unique index appears to be transactional there)
pkg/sql/opt/optbuilder/scope.go, line 530 at r1 (raw file):
// colList returns a ColList of all the columns in this scope, // excluding orderByCols.
I think we changed this to extraCols (would be great to change the comment above, too)
pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 935 at r3 (raw file):
b INT, UNIQUE WITHOUT INDEX (k) WHERE b > 0, UNIQUE WITHOUT INDEX (k, a) WHERE b > 0
I think this test would be a bit more useful if (like the test for full constraints) you have a 2-column primary key, and you have constraints that include, none, one, or both of the pk columns.
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
from
e94cdf2 to
1a47d15
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/unique, line 79 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
How about adding another partial unique without index constraint where the predicate contains the unique column?
Done.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1209 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] constraints -> constraint
Done.
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 285 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
My guiding principle with this uniqueness stuff has been to try to make the unique constraints behave as close as possible to unique indexes. So I did a bit of testing, and it seems to me like we shouldn't need to include mutation columns here (yet). Here was my test:
[email protected]:26257/movr> create table t (k int primary key, v int unique); CREATE TABLE Time: 3ms total (execution 3ms / network 0ms) [email protected]:26257/movr> begin; BEGIN Time: 0ms total (execution 0ms / network 0ms) [email protected]:26257/movr OPEN> alter table t add column w int not null default 0; ALTER TABLE Time: 2ms total (execution 2ms / network 0ms) [email protected]:26257/movr OPEN> create unique index w_idx on t (w); CREATE INDEX Time: 5ms total (execution 5ms / network 0ms) [email protected]:26257/movr OPEN> insert into t values (3, 3), (4, 4); INSERT 2 Time: 2ms total (execution 1ms / network 0ms) [email protected]:26257/movr OPEN> commit; ERROR: transaction committed but schema change aborted with error: (23505): failed to ingest index entries during backfill: duplicate key value violates unique constraint "w_idx" SQLSTATE: XXA00 DETAIL: Key (w)=(0) already exists. CONSTRAINT: w_idx HINT: Some of the non-DDL statements may have committed successfully, but some of the DDL statement(s) failed. Manual inspection may be required to determine the actual state of the database. -- See: https://github.com/cockroachdb/cockroach/issues/42061 [email protected]:26257/movr> select * from t; k | v ----+---- 3 | 3 4 | 4 (2 rows) Time: 1ms total (execution 1ms / network 0ms) [email protected]:26257/movr> truncate t; TRUNCATE Time: 18ms total (execution 5ms / network 13ms) [email protected]:26257/movr> SET experimental_enable_unique_without_index_constraints = true; SET Time: 0ms total (execution 0ms / network 0ms) [email protected]:26257/movr> begin; BEGIN Time: 0ms total (execution 0ms / network 0ms) [email protected]:26257/movr OPEN> alter table t add column w int not null default 0; ALTER TABLE Time: 2ms total (execution 2ms / network 0ms) [email protected]:26257/movr OPEN> alter table t add constraint w_no_idx unique without index (w); ALTER TABLE Time: 5ms total (execution 5ms / network 0ms) [email protected]:26257/movr OPEN> insert into t values (3, 3), (4, 4); INSERT 2 Time: 1ms total (execution 1ms / network 0ms) [email protected]:26257/movr OPEN> commit; ERROR: transaction committed but schema change aborted with error: (23505): could not create unique constraint "w_no_idx" SQLSTATE: XXA00 DETAIL: Key (w)=(0) is duplicated. CONSTRAINT: w_no_idx HINT: Some of the non-DDL statements may have committed successfully, but some of the DDL statement(s) failed. Manual inspection may be required to determine the actual state of the database. -- See: https://github.com/cockroachdb/cockroach/issues/42061 [email protected]:26257/movr> select * from t; k | v ----+---- 3 | 3 4 | 4 (2 rows) Time: 1ms total (execution 1ms / network 0ms)Notice that both the schema changes and insertions appear to succeed during the transactions both with and without indexes, and then the schema changes fail at commit time. This will obviously change once schema changes are transactional, and at that point maybe we will need to include mutation columns here.
Does that seem reasonable? If so, can you add a TODO?
(Note that this is different behavior from Postgres, since adding a unique index appears to be transactional there)
SGTM. Added a TODO.
pkg/sql/opt/optbuilder/scope.go, line 530 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I think we changed this to
extraCols(would be great to change the comment above, too)
Done.
pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 935 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I think this test would be a bit more useful if (like the test for full constraints) you have a 2-column primary key, and you have constraints that include, none, one, or both of the pk columns.
Done.
There was a problem hiding this comment.
Reviewed 17 of 26 files at r4, 9 of 9 files at r5.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder.go, line 1324 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Why can't you just use
inputColMeta.AliasandinputColMeta.Typeas the name/type? Are these differences in name/type relevant when there is anASclause?
Don't you want the alias instead of the table column? Seems like a lot of tests changed as a result of this, which doesn't seem ideal.
As a separate matter, we probably want to update the alias so that it uses the underlying table column when it makes sense (instead of "column2", for example), but that would happen elsewhere in the code.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1209 at r3 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Done.
Now they are both plural... but I think you want singular here 🙂
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
from
1a47d15 to
25e99ac
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder.go, line 1324 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Don't you want the alias instead of the table column? Seems like a lot of tests changed as a result of this, which doesn't seem ideal.
As a separate matter, we probably want to update the alias so that it uses the underlying table column when it makes sense (instead of "column2", for example), but that would happen elsewhere in the code.
Hmmm, reviewable ate my original response. It was something like
"The unique constraint predicate refers to table columns, not the WithScan input columns (which can have synthesized names like column1), so the names of the WithScan output columns must be the names of the columns in the table. If we use the input column aliases then we can't build a predicate expression to filter the WithScan columns."
I explained this briefly in the commit messages.
Originally I had the scopeColumns using the table's column names, but the metadata had the alias, but this was confusing and seemed incorrect to me.
pkg/sql/opt/optbuilder/mutation_builder.go, line 1209 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Now they are both plural... but I think you want singular here 🙂
oops, done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder.go, line 1324 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Hmmm, reviewable ate my original response. It was something like
"The unique constraint predicate refers to table columns, not the WithScan input columns (which can have synthesized names like
column1), so the names of the WithScan output columns must be the names of the columns in the table. If we use the input column aliases then we can't build a predicate expression to filter the WithScan columns."I explained this briefly in the commit messages.
Originally I had the scopeColumns using the table's column names, but the metadata had the alias, but this was confusing and seemed incorrect to me.
Ah ok makes sense. Can you update the comment here to include some of that info? Thanks!
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
from
25e99ac to
6b0b3f9
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder.go, line 1324 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Ah ok makes sense. Can you update the comment here to include some of that info? Thanks!
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 17 of 26 files at r4, 8 of 9 files at r5, 1 of 1 files at r7.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_fk.go, line 674 at r4 (raw file):
// (a IS NOT NULL) AND (b IS NOT NULL) ... filters := make(memo.FiltersExpr, 0, numCols-notNullWithScanCols.Len()) for _, col := range withScanScope.colList() {
[nit] We can just iterate on the cols directly
pkg/sql/opt/optbuilder/mutation_builder_fk.go, line 697 at r4 (raw file):
// (a IS NOT NULL) OR (b IS NOT NULL) ... var condition opt.ScalarExpr for _, col := range withScanScope.colList() {
[nit] We can just iterate on the cols directly
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 284 at r7 (raw file):
func (h *uniqueCheckHelper) buildTableScan() (outScope *scope, ordinals []int) { tabMeta := h.mb.b.addTable(h.mb.tab, tree.NewUnqualifiedTableName(h.mb.tab.Name())) // TODO(mgartner): Include mutation columns once schema changes are
I don't understand this comment.
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
from
6b0b3f9 to
b3b0143
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_fk.go, line 674 at r4 (raw file):
Previously, RaduBerinde wrote…
[nit] We can just iterate on the cols directly
Done.
pkg/sql/opt/optbuilder/mutation_builder_fk.go, line 697 at r4 (raw file):
Previously, RaduBerinde wrote…
[nit] We can just iterate on the cols directly
Done.
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 284 at r7 (raw file):
Previously, RaduBerinde wrote…
I don't understand this comment.
I rewrote it, LMK if that helps.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 284 at r7 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
I rewrote it, LMK if that helps.
Such a case should first add the columns, and then add the constraint (at which point the column shouldn't show up as mutation columns). We can't rely on a value read from a mutation column, so it's usually wrong to scan it.
I think I'd remove the comment altogether.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 284 at r7 (raw file):
Previously, RaduBerinde wrote…
Such a case should first add the columns, and then add the constraint (at which point the column shouldn't show up as mutation columns). We can't rely on a value read from a mutation column, so it's usually wrong to scan it.
I think I'd remove the comment altogether.
Ahh makes sense. cc @rytaft for context, since we discussed this earlier.
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
from
b3b0143 to
65feb05
Compare
There was a problem hiding this comment.
Reviewed 1 of 10 files at r8, 9 of 9 files at r9.
Reviewable status:
pkg/sql/opt/optbuilder/mutation_builder_unique.go, line 284 at r7 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Ahh makes sense. cc @rytaft for context, since we discussed this earlier.
👍
|
bors r+ |
|
Build failed (retrying...): |
This commit updates `mutationBuilder.buildCheckInputScan` to build a scope containing the constructed WithScan expression and the scope columns output by the expression. Also, the WithScan output column names are now the names of the columns in the underlying table, rather than the names of the input columns. Access to a scope for the WithScan expression will be required to support partial `UNIQUE WITHOUT INDEX` constraints. For uniqueness checks, a filter on the WithScan columns must be added to the semi-join filters. The WithScan scope will be required in order to build the filter expression. Additionally, the names of the WithScan output columns must match the names of the columns in the underlying table in order to build partial predicate expressions that refer to the table's columns. Release note: None
|
bors r- |
|
Canceled. |
Uniqueness checks on INSERTs are now performed for partial UNIQUE WITHOUT INDEX constraints. The semi-join filters in the uniqueness checks filter out rows that do not satisfy the predicate. Rows on both sides of the join are filtered. This is required to prevent duplicate key violations from occurring for rows that do not satisfy the predicate of the partial unique constraint. The WithScan and Scan of the uniqueness check now produce all ordinary table columns, rather than just the PK and unique columns, because predicates may refer any columns in the table. Informs cockroachdb#59195 There is no release note because these constraints are gated behind the experimental_enable_unique_without_index_constraints session variable. Release note: None
mgartner
force-pushed
the
plan-partial-unique-without-index
branch
from
65feb05 to
16d927c
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
This commit add uniqueness checks for partial `UNIQUE WITHOUT INDEX` constraints during `UPDATE` statements. As partial of this change, I discovered that cockroachdb#60535 introduced a regression where columns not required by uniqueness checks are not pruned. I've left TODOs in the column pruning tests and plan on fixing this in a follow-up PR. There is no release note because these constraints are gated behind the experimental_enable_unique_without_index_constraints session variable. Release note: None
This commit add uniqueness checks for partial `UNIQUE WITHOUT INDEX` constraints during `UPDATE` statements. As partial of this change, I discovered that cockroachdb#60535 introduced a regression where columns not required by uniqueness checks are not pruned. I've left TODOs in the column pruning tests and plan on fixing this in a follow-up PR. There is no release note because these constraints are gated behind the experimental_enable_unique_without_index_constraints session variable. Release note: None
60836: opt: support UPDATE with partial UNIQUE WITHOUT INDEX constraints r=mgartner a=mgartner This commit add uniqueness checks for partial `UNIQUE WITHOUT INDEX` constraints during `UPDATE` statements. As partial of this change, I discovered that #60535 introduced a regression where columns not required by uniqueness checks are not pruned. I've left TODOs in the column pruning tests and plan on fixing this in a follow-up PR. There is no release note because these constraints are gated behind the experimental_enable_unique_without_index_constraints session variable. Release note: None 60992: kv: make RaftCommand.ClosedTimestamp nullable r=nvanbenschoten a=nvanbenschoten Fixes #60852. Fixes #60833. Fixes #58298. Fixes #59428. Fixes #60756. Fixes #60848. Fixes #60849. In #60852 and related issues, we saw that the introduction of a non-nullable `RaftCommand.ClosedTimestamp`, coupled with the `ClosedTimestampFooter` encoding strategy we use, led to encoded `RaftCommand` protos with their ClosedTimestamp field set twice. This is ok from a correctness perspective, at least as protobuf is concerned, but it led to a subtle interaction where the process of passing through sideloading (`maybeInlineSideloadedRaftCommand(maybeSideloadEntriesImpl(e))`) would reduce the size of an encoded RaftCommand by 3 bytes (the encoded size of an empty `hlc.Timestamp`). This was resulting in an `uncommittedSize` leak in Raft, which was eventually stalling on its `MaxUncommittedEntriesSize` limit. This commit fixes this issue by making `RaftCommand.ClosedTimestamp` nullable. With the field marked as nullable, it will no longer be encoded as an empty timestamp when unset, ensuring that when the encoded `ClosedTimestampFooter` is appended, it contains the only instance of the `ClosedTimestamp` field. cc. @cockroachdb/bulk-io Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]>
optbuilder: return scope from mutationBuilder.buildCheckInputScan
This commit updates
mutationBuilder.buildCheckInputScanto build ascope containing the constructed WithScan expression and the scope
columns output by the expression. Also, the WithScan output column names
are now the names of the columns in the underlying table, rather than
the names of the input columns.
Access to a scope for the WithScan expression will be required to
support partial
UNIQUE WITHOUT INDEXconstraints. For uniquenesschecks, a filter on the WithScan columns must be added to the semi-join
filters. The WithScan scope will be required in order to build the
filter expression. Additionally, the names of the WithScan output
columns must match the names of the columns in the underlying table in
order to build partial predicate expressions that refer to the table's
columns.
Release note: None
sql: support INSERT with partial UNIQUE WITHOUT INDEX constraints
Uniqueness checks on INSERTs are now performed for partial UNIQUE
WITHOUT INDEX constraints. The semi-join filters in the uniqueness
checks filter out rows that do not satisfy the predicate. Rows on both
sides of the join are filtered. This is required to prevent duplicate
key violations from occurring for rows that do not satisfy the predicate
of the partial unique constraint.
The WithScan and Scan of the uniqueness check now produce all ordinary
table columns, rather than just the PK and unique columns, because
predicates may refer any columns in the table.
Informs #59195
There is no release note because these constraints are gated behind the
experimental_enable_unique_without_index_constraints session variable.
Release note: None