Skip to content

Commit

Permalink
Merge cockroachdb#83222
Browse files Browse the repository at this point in the history 
83222: sql/schemachanger: model IndexColumn, avoid index backfill when unneeded r=ajwerner a=ajwerner

**sql/schemachanger/rel: add support for Neq (!=)**

**sql/schemachanger: model IndexColumn explicitly**
This change normalizes the columns in an index. This lays the foundation to
handle cases where we add a NULL-able new column without a default value. In
these cases, we should just modify the existing index in-place. That work will
come in a follow-up commit. This will also prove valuable for more complex
add column scenarios.

**sql/schemachanger: add a special case for simple add column**
Before this change, when we added a column, we'd always create a new primary
index and swap into it. In the case where we're adding a nullable column with
no default value or computed expression, we have no reason to do this whole
dance. Indeed, backfilling that new index was a major regression for this
common case when compared to the legacy schema changer.

In this commit, we detect this special case and just add the new column to
the existing primary index.

Release note: None

Co-authored-by: Andrew Werner <[email protected]>
  • Loading branch information
@ajwerner
craig[bot] and ajwerner committed Jun 28, 2022
2 parents 615051e + 0c3f215 commit e19d98d
Show file tree
Hide file tree
Showing 127 changed files with 4,559 additions and 2,552 deletions.
27 changes: 18 additions & 9 deletions pkg/ccl/changefeedccl/changefeed_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6311,14 +6311,25 @@ func TestChangefeedPredicateWithSchemaChange(t *testing.T) {
}

for _, tc := range []testCase{
// The default policy is to skip schema changes which add new columns which
{
name: "add column",
name: "add column no default",
createFeedStmt: "CREATE CHANGEFEED AS SELECT * FROM foo",
initialPayload: initialPayload,
alterStmt: "ALTER TABLE foo ADD COLUMN new STRING",
afterAlterStmt: "INSERT INTO foo (a, b) VALUES (3, 'tres')",
payload: []string{
`foo: [3, "tres"]->{"after": {"a": 3, "b": "tres", "c": null, "e": "inactive", "new": null}}`,
},
},
{
name: "add column",
createFeedStmt: "CREATE CHANGEFEED AS SELECT * FROM foo",
initialPayload: initialPayload,
alterStmt: "ALTER TABLE foo ADD COLUMN new STRING DEFAULT 'new'",
payload: []string{
`foo: [1, "one"]->{"after": {"a": 1, "b": "one", "c": null, "e": "inactive", "new": null}}`,
`foo: [2, "two"]->{"after": {"a": 2, "b": "two", "c": "c string", "e": "open", "new": null}}`,
`foo: [1, "one"]->{"after": {"a": 1, "b": "one", "c": null, "e": "inactive", "new": "new"}}`,
`foo: [2, "two"]->{"after": {"a": 2, "b": "two", "c": "c string", "e": "open", "new": "new"}}`,
},
},
{
Expand All @@ -6328,11 +6339,11 @@ func TestChangefeedPredicateWithSchemaChange(t *testing.T) {
name: "add alt.status",
createFeedStmt: "CREATE CHANGEFEED AS SELECT * FROM foo",
initialPayload: initialPayload,
alterStmt: "ALTER TABLE foo ADD COLUMN alt alt.status",
alterStmt: "ALTER TABLE foo ADD COLUMN alt alt.status DEFAULT 'alt_closed'",
afterAlterStmt: "INSERT INTO foo (a, b, alt) VALUES (3, 'tres', 'alt_open')",
payload: []string{
`foo: [1, "one"]->{"after": {"a": 1, "alt": null, "b": "one", "c": null, "e": "inactive"}}`,
`foo: [2, "two"]->{"after": {"a": 2, "alt": null, "b": "two", "c": "c string", "e": "open"}}`,
`foo: [1, "one"]->{"after": {"a": 1, "alt": "alt_closed", "b": "one", "c": null, "e": "inactive"}}`,
`foo: [2, "two"]->{"after": {"a": 2, "alt": "alt_closed", "b": "two", "c": "c string", "e": "open"}}`,
`foo: [3, "tres"]->{"after": {"a": 3, "alt": "alt_open", "b": "tres", "c": null, "e": "inactive"}}`,
},
},
Expand Down Expand Up @@ -6417,9 +6428,7 @@ func TestChangefeedPredicateWithSchemaChange(t *testing.T) {
},
} {
t.Run(tc.name, func(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
cdcTest(t, testFn(tc), feedTestEnterpriseSinks)
})
cdcTest(t, testFn(tc), feedTestEnterpriseSinks)
})
}
}
Expand Down
20 changes: 2 additions & 18 deletions pkg/ccl/kvccl/kvfollowerreadsccl/testdata/boundedstaleness/single_row
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,40 +241,24 @@ query idx=2
SELECT * FROM t2 AS OF SYSTEM TIME with_min_timestamp(now() - '10s', true) WHERE pk = 2
----
pq: referenced descriptor ID 105: descriptor not found
events (15 found):
events (7 found):
* event 1: colbatchscan trace on node_idx 2: local read
* event 2: transaction retry on node_idx: 2
* event 3: colbatchscan trace on node_idx 2: local read
* event 4: transaction retry on node_idx: 2
* event 5: colbatchscan trace on node_idx 2: local read
* event 6: transaction retry on node_idx: 2
* event 7: colbatchscan trace on node_idx 2: local read
* event 8: transaction retry on node_idx: 2
* event 9: colbatchscan trace on node_idx 2: local read
* event 10: transaction retry on node_idx: 2
* event 11: colbatchscan trace on node_idx 2: local read
* event 12: transaction retry on node_idx: 2
* event 13: colbatchscan trace on node_idx 2: local read
* event 14: transaction retry on node_idx: 2
* event 15: colbatchscan trace on node_idx 2: local read

query idx=2
SELECT * FROM t2 AS OF SYSTEM TIME with_min_timestamp(now() - '10s', true) WHERE pk = 2
----
pq: referenced descriptor ID 105: descriptor not found
events (15 found):
events (7 found):
* event 1: colbatchscan trace on node_idx 2: local read
* event 2: transaction retry on node_idx: 2
* event 3: colbatchscan trace on node_idx 2: local read
* event 4: transaction retry on node_idx: 2
* event 5: colbatchscan trace on node_idx 2: local read
* event 6: transaction retry on node_idx: 2
* event 7: colbatchscan trace on node_idx 2: local read
* event 8: transaction retry on node_idx: 2
* event 9: colbatchscan trace on node_idx 2: local read
* event 10: transaction retry on node_idx: 2
* event 11: colbatchscan trace on node_idx 2: local read
* event 12: transaction retry on node_idx: 2
* event 13: colbatchscan trace on node_idx 2: local read
* event 14: transaction retry on node_idx: 2
* event 15: colbatchscan trace on node_idx 2: local read
1 change: 1 addition & 0 deletions pkg/ccl/partitionccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ go_test(
"//pkg/sql",
"//pkg/sql/catalog",
"//pkg/sql/catalog/bootstrap",
"//pkg/sql/catalog/catalogkeys",
"//pkg/sql/catalog/descbuilder",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/desctestutils",
Expand Down
3 changes: 2 additions & 1 deletion pkg/ccl/partitionccl/zone_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
Expand Down Expand Up @@ -314,7 +315,7 @@ func TestGenerateSubzoneSpans(t *testing.T) {
directions := []encoding.Direction{encoding.Ascending /* index ID */}
for i := 0; i < idx.NumKeyColumns(); i++ {
cd := idx.GetKeyColumnDirection(i)
ed, err := cd.ToEncodingDirection()
ed, err := catalogkeys.IndexColumnEncodingDirection(cd)
if err != nil {
t.Fatal(err)
}
Expand Down
82 changes: 56 additions & 26 deletions pkg/ccl/schemachangerccl/testdata/decomp/multiregion
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,14 @@ ElementState:
Status: PUBLIC
- PrimaryIndex:
embeddedIndex:
compositeColumnIds: []
constraintId: 1
indexId: 1
isConcurrently: false
isCreatedExplicitly: false
isInverted: false
isUnique: true
keyColumnDirections:
- ASC
keyColumnIds:
- 2
keySuffixColumnIds: []
sharding: null
sourceIndexId: 0
storingColumnIds:
- 1
tableId: 110
temporaryIndexId: 0
Status: PUBLIC
Expand Down Expand Up @@ -280,6 +272,22 @@ ElementState:
name: table_global_pkey
tableId: 110
Status: PUBLIC
- IndexColumn:
columnId: 1
direction: ASC
indexId: 1
kind: STORED
ordinalInKind: 0
tableId: 110
Status: PUBLIC
- IndexColumn:
columnId: 2
direction: ASC
indexId: 1
kind: KEY
ordinalInKind: 0
tableId: 110
Status: PUBLIC
- Namespace:
databaseId: 104
descriptorId: 110
Expand Down Expand Up @@ -361,22 +369,14 @@ ElementState:
Status: PUBLIC
- PrimaryIndex:
embeddedIndex:
compositeColumnIds: []
constraintId: 1
indexId: 1
isConcurrently: false
isCreatedExplicitly: false
isInverted: false
isUnique: true
keyColumnDirections:
- ASC
keyColumnIds:
- 2
keySuffixColumnIds: []
sharding: null
sourceIndexId: 0
storingColumnIds:
- 1
tableId: 109
temporaryIndexId: 0
Status: PUBLIC
Expand Down Expand Up @@ -526,6 +526,22 @@ ElementState:
name: table_regional_by_table_pkey
tableId: 109
Status: PUBLIC
- IndexColumn:
columnId: 1
direction: ASC
indexId: 1
kind: STORED
ordinalInKind: 0
tableId: 109
Status: PUBLIC
- IndexColumn:
columnId: 2
direction: ASC
indexId: 1
kind: KEY
ordinalInKind: 0
tableId: 109
Status: PUBLIC
- Namespace:
databaseId: 104
descriptorId: 109
Expand Down Expand Up @@ -617,24 +633,14 @@ ElementState:
Status: PUBLIC
- PrimaryIndex:
embeddedIndex:
compositeColumnIds: []
constraintId: 1
indexId: 1
isConcurrently: false
isCreatedExplicitly: false
isInverted: false
isUnique: true
keyColumnDirections:
- ASC
- ASC
keyColumnIds:
- 3
- 1
keySuffixColumnIds: []
sharding: null
sourceIndexId: 0
storingColumnIds:
- 2
tableId: 108
temporaryIndexId: 0
Status: PUBLIC
Expand Down Expand Up @@ -868,6 +874,30 @@ ElementState:
indexId: 1
tableId: 108
Status: PUBLIC
- IndexColumn:
columnId: 1
direction: ASC
indexId: 1
kind: KEY
ordinalInKind: 1
tableId: 108
Status: PUBLIC
- IndexColumn:
columnId: 2
direction: ASC
indexId: 1
kind: STORED
ordinalInKind: 0
tableId: 108
Status: PUBLIC
- IndexColumn:
columnId: 3
direction: ASC
indexId: 1
kind: KEY
ordinalInKind: 0
tableId: 108
Status: PUBLIC
- Namespace:
databaseId: 104
descriptorId: 108
Expand Down
Loading

0 comments on commit e19d98d

Please sign in to comment.