sql: alter primary key is not idempotent #61345
Conversation
|
This change is |
fqazi
force-pushed
the
alterIdempotent
branch
3 times, most recently
from
633fb6c to
d8704f5
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/ccl/backupccl/backup_test.go, line 832 at r1 (raw file):
sqlDB.Exec(t, `CREATE UNIQUE INDEX id2 ON data.bank (id)`) // This statement will be a no-op since the primary key is already ID sqlDB.Exec(t, `ALTER TABLE data.bank ALTER PRIMARY KEY USING COLUMNS(id)`)
I think this test has actually rotted a bit and the way that we create data.bank has changed under it. Let's remove this ALTER TABLE ... PK entirely.
pkg/ccl/backupccl/backup_test.go, line 842 at r1 (raw file):
t.Fatalf("expected %d rows, got %d", numAccounts, exportedRows) } expectedIndexEntries := numAccounts * 2 // PK and old secondary idx
This comment has also rotted, PK actually isn't included in this count. I think the 2 indexes are the old secondary idx (balance_idx) and the new index id2. Can we update this comment to: // Indexes id2 and balance_idx? Thanks!
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/backupccl/backup_test.go, line 832 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
I think this test has actually rotted a bit and the way that we create
data.bankhas changed under it. Let's remove this ALTER TABLE ... PK entirely.
Done.
pkg/ccl/backupccl/backup_test.go, line 842 at r1 (raw file):
Previously, pbardea (Paul Bardea) wrote…
This comment has also rotted, PK actually isn't included in this count. I think the 2 indexes are the old secondary idx (balance_idx) and the new index
id2. Can we update this comment to:// Indexes id2 and balance_idx? Thanks!
Done.
There was a problem hiding this comment.
Something seems off with something related to the multi-region code. I have a feeling it relates to
cockroach/pkg/sql/alter_table_locality.go
Line 420 in fba7ed1
Reviewable status:
There was a problem hiding this comment.
Yes, that case is broken, need additional logic to handle it.
Reviewable status:
There was a problem hiding this comment.
Reviewed 1 of 3 files at r1, 1 of 2 files at r2.
Reviewable status:
pkg/sql/alter_primary_key.go, line 534 at r2 (raw file):
alterPKNode *tree.AlterTableAlterPrimaryKey, alterPrimaryKeyLocalitySwap *alterPrimaryKeyLocalitySwap, ) (bool, error) {
nit: name returned booleans, I find it aids readability. You can use _ for the error.
pkg/sql/alter_primary_key.go, line 566 at r2 (raw file):
} if len(oldPK.IndexDesc().Interleave.Ancestors) == 0 {
nit: things wouldn't be so long if you just assigned oldPK.IndexDesc().Interleave.Ancestors to a variable.
pkg/sql/alter_primary_key.go, line 607 at r2 (raw file):
// then recreate indexes. if alterPrimaryKeyLocalitySwap != nil { if !alterPrimaryKeyLocalitySwap.localityConfigSwap.NewLocalityConfig.Equal(alterPrimaryKeyLocalitySwap.localityConfigSwap.OldLocalityConfig) {
nit: how about some line wrapping or perhaps some variables to make this more readable.
|
Thanks for the review! |
|
bors r+ |
Fixes: cockroachdb#59307 Previously, issuing an alter primary key with the exact same definition as the current primary key would cause expensive indexes to be recreated even when there was no logical change. There was overhead involved in creating these indexes and as a side effect existing indexes for the primary key would also get needlessly renamed. To address this, this patch uses the AST node for alter primary key to determine if the requested change is logically the same before any operation is executed. If its logically the same then it becomes a no-op. Release justification: This is a low risk change with good benefit to the user base by reducing extra indexes getting made and rename operations. Release note (bug fix): Alter primary key was not idempotent, so logical equivalent changes to primary keys would unnecessarily create new indexes.
|
Canceled. |
|
bors r+ |
|
Build succeeded: |
The code in cockroachdb#61345 added a test that, under stress, nicely revealed a bug. The bug is that we're mutating the AST in place and then, on retries, we hit an error. The fix is to not mutate the AST. I wish I had a more comprehensive testing strategy to ensure this didn't happen. On some level, we could clone the AST when passing it to various DDL plan node constructors. That's very defensive but also probably fine. Another thing that would be cool would be to just assert that after planning the AST did not change. Touches cockroachdb#60824. Release note: None
The code in cockroachdb#61345 added a test that, under stress, nicely revealed a bug. The bug is that we're mutating the AST in place and then, on retries, we hit an error. The fix is to not mutate the AST. I wish I had a more comprehensive testing strategy to ensure this didn't happen. On some level, we could clone the AST when passing it to various DDL plan node constructors. That's very defensive but also probably fine. Another thing that would be cool would be to just assert that after planning the AST did not change. Touches cockroachdb#60824. Release note: None
The code in cockroachdb#61345 added a test that, under stress, nicely revealed a bug. The bug is that we're mutating the AST in place and then, on retries, we hit an error. The fix is to not mutate the AST. I wish I had a more comprehensive testing strategy to ensure this didn't happen. On some level, we could clone the AST when passing it to various DDL plan node constructors. That's very defensive but also probably fine. Another thing that would be cool would be to just assert that after planning the AST did not change. Touches cockroachdb#60824. Release note: None
61784: sql: stop mutating AST in AlterPrimaryKey r=fqazi a=ajwerner The code in #61345 added a test that, under stress, nicely revealed a bug. The bug is that we're mutating the AST in place and then, on retries, we hit an error. The fix is to not mutate the AST. I wish I had a more comprehensive testing strategy to ensure this didn't happen. On some level, we could clone the AST when passing it to various DDL plan node constructors. That's very defensive but also probably fine. Another thing that would be cool would be to just assert that after planning the AST did not change. Touches #60824. Release note: None 61791: kv/kvserver: skip TestEagerReplication r=RaduBerinde a=adityamaru Refs: #54646 Reason: flaky test Generated by bin/skip-test. Release justification: non-production code changes Release note: None 61796: bazel: make `pkg/internal/rsg/yacc` test runnable in Bazel r=rickystewart a=rickystewart This test was missing a dependency on `sql.y`. Release note: None 61797: bazel: make `pkg/sql/parser` test runnable in Bazel r=rickystewart a=rickystewart There was a typo in the `genrule` definition here. Release note: None Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Aditya Maru <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
Fixes: #59307
Previously, issuing an alter primary key with the exact same
definition as the current primary key would cause expensive
indexes to be recreated even when there was no logical change.
There was overhead involved in creating these indexes and as
a side effect existing indexes for the primary key would also
get needlessly renamed. To address this, this patch uses the
AST node for alter primary key to determine if the requested
change is logically the same before any operation is executed.
If its logically the same then it becomes a no-op.
Release justification: This is a low risk change with good benefit
to the user base by reducing extra indexes getting made and rename
operations.
Release note (bug fix): Alter primary key was not idempotent, so
logical equivalent changes to primary keys would unnecessarily
create new indexes.