Skip to content

Commit a119b32

Browse files
rafissrafiss
committed
schemachanger: fix EXPLAIN output of storage param operations
Since the protobuf was embedded into the scop for mutating storage parameters, it was not being rendered properly in EXPLAIN output. Fix it by making an explicit field instead. Release note: None
1 parent bbc06e5 commit a119b32

File tree

8 files changed

+234
-33
lines changed

8 files changed

+234
-33
lines changed

‎pkg/sql/GEMINI.md‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
# SQL Package Development Guide
2+
3+
## Schema Changers: Legacy vs Declarative
4+
5+
CockroachDB has two schema change systems: the legacy schema changer and the newer declarative schema changer. Understanding their differences is crucial for DDL development work.
6+
7+
### Architectural Overview
8+
9+
**Legacy Schema Changer** (`/pkg/sql/schema_changer.go`):
10+
- **Hard-coded state transitions**: Uses fixed sequences of descriptor mutations stored in the `mutations` slice
11+
- **Imperative approach**: Logic written procedurally with specific code paths for each DDL operation
12+
- **Limited state model**: Uses states like `DELETE_ONLY`, `WRITE_ONLY`, `BACKFILL_ONLY` from `DescriptorMutation`
13+
- **Job type**: Uses `SCHEMA_CHANGE` and `TYPEDESC_SCHEMA_CHANGE` job types
14+
15+
**Declarative Schema Changer** (`/pkg/sql/schemachanger/`):
16+
- **Element-based modeling**: Schema changes modeled as elements (columns, indexes, constraints) with target statuses
17+
- **Declarative planning**: Uses rules and dependency graphs to generate execution plans
18+
- **State stored in descriptors**: Uses `declarative_schema_changer_state` field instead of `mutations` slice
19+
- **Job type**: Uses `NEW_SCHEMA_CHANGE` job type
20+
21+
### State Management Differences
22+
23+
**Legacy Schema Changer:**
24+
```go
25+
// Uses DescriptorMutation in mutations slice
26+
type DescriptorMutation struct {
27+
State DescriptorMutation_State // DELETE_ONLY, WRITE_ONLY, etc.
28+
Direction DescriptorMutation_Direction // ADD, DROP
29+
// ... specific column/index descriptors
30+
}
31+
```
32+
33+
**Declarative Schema Changer:**
34+
```proto
35+
// Uses element model with target statuses
36+
message Target {
37+
ElementProto element_proto = 1;
38+
Status target_status = 3; // PUBLIC, ABSENT, TRANSIENT_ABSENT
39+
}
40+
```
41+
42+
### Element Model (Declarative)
43+
44+
Elements are defined in `/pkg/sql/schemachanger/scpb/elements.proto`:
45+
- **Table-level**: `Table`, `TableComment`, `TableData`
46+
- **Column elements**: `Column`, `ColumnName`, `ColumnType`, `ColumnDefaultExpression`
47+
- **Index elements**: `PrimaryIndex`, `SecondaryIndex`, `IndexColumn`
48+
- **Constraint elements**: `CheckConstraint`, `ForeignKeyConstraint`
49+
50+
Status transitions: `ABSENT` -> `DELETE_ONLY` -> `WRITE_ONLY` -> `BACKFILL_ONLY` -> `VALIDATED` -> `PUBLIC`
51+
52+
### Planning and Execution
53+
54+
**Legacy Schema Changer:**
55+
- Planning and execution tightly coupled
56+
- Each DDL statement has custom imperative logic
57+
- State transitions are hard-coded sequences
58+
- Difficult to handle complex multi-statement transactions
59+
60+
**Declarative Schema Changer:**
61+
- Clear separation between planning and execution phases
62+
- Uses dependency graphs and rules (`/pkg/sql/schemachanger/scplan/internal/rules/`)
63+
- Can handle complex multi-statement transactions declaratively
64+
- Better composition of multiple DDL operations
65+
66+
### Error Handling and Rollback
67+
68+
**Legacy Schema Changer:**
69+
- Known rollback issues, especially with data loss during failed multi-operation schema changes
70+
- Example: dropping columns alongside adding indexes can permanently lose data on rollback
71+
72+
**Declarative Schema Changer:**
73+
- Designed for correct rollbacks by reversing target statuses
74+
- Uses `OnFailOrCancel` method to revert by flipping directions (ADD becomes DROP)
75+
- Better handling of complex failure scenarios
76+
77+
### Configuration
78+
79+
Enable declarative schema changer via:
80+
```sql
81+
-- Session level
82+
SET use_declarative_schema_changer = 'on'; -- Options: 'off', 'on', 'unsafe', 'unsafe_always'
83+
84+
-- Cluster-wide default
85+
SET CLUSTER SETTING sql.defaults.use_declarative_schema_changer = 'on';
86+
```
87+
88+
### Development Patterns
89+
90+
**Adding New DDL Operations:**
91+
92+
*Legacy Schema Changer:*
93+
- Add case statements in main schema changer loop
94+
- Implement specific state transition logic
95+
- Hard-code mutation sequences
96+
97+
*Declarative Schema Changer:*
98+
- Define new elements in `elements.proto`
99+
- Add rules for state transitions in `scplan/internal/rules/`
100+
- Implement operations in `scop/` and execution in `scexec/`
101+
- More modular and composable approach
102+
103+
### Code Organization
104+
105+
**Legacy Schema Changer:**
106+
- `/pkg/sql/schema_changer.go` - Main implementation
107+
- `/pkg/sql/schema_changer_state.go` - State management
108+
- `/pkg/sql/type_change.go` - Type changes
109+
110+
**Declarative Schema Changer:**
111+
- `scpb/` - Protocol buffer definitions for elements and state
112+
- `scbuild/` - Building targets from DDL ASTs
113+
- `scplan/` - Planning state transitions and dependencies
114+
- `scexec/` - Executing operations
115+
- `scop/` - Operation definitions
116+
- `screl/` - Relational model for elements
117+
118+
### When to Use Each
119+
120+
**Legacy Schema Changer:**
121+
- Still required for some DDL operations not yet implemented in declarative changer
122+
- May be faster for simple, single-operation schema changes
123+
- More predictable performance due to hard-coded paths
124+
125+
**Declarative Schema Changer:**
126+
- Better for complex multi-statement transactions
127+
- More overhead in planning phase but better execution for complex scenarios
128+
- Required for future transactional schema changes
129+
- Better correctness guarantees and rollback handling
130+
131+
### Migration Path
132+
133+
CockroachDB is gradually migrating from legacy to declarative:
134+
1. **Coexistence period**: Both schema changers run simultaneously
135+
2. **Feature-by-feature migration**: DDL operations migrated individually
136+
3. **Version compatibility**: Old jobs must continue during upgrades
137+
4. **Extensive testing**: Test coverage in `schemachanger_test.go` and related files
138+
139+
### Key Benefits of Declarative Approach
140+
141+
1. **Correctness**: Better handling of complex scenarios and rollbacks
142+
2. **Extensibility**: Easier to add new DDL operations
143+
3. **Composability**: Can handle multiple DDL operations in single transaction
144+
4. **Testing**: Better separation of concerns allows more targeted testing
145+
5. **Future-ready**: Foundation for transactional schema changes
146+
147+
The declarative schema changer represents a significant architectural improvement addressing fundamental limitations of the legacy approach, particularly around correctness, extensibility, and complex transaction handling.

‎pkg/sql/schemachanger/scexec/scmutationexec/table.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,23 +35,23 @@ func (i *immediateVisitor) SetTableSchemaLocked(
3535
func (i *immediateVisitor) SetTableStorageParam(
3636
ctx context.Context, op scop.SetTableStorageParam,
3737
) error {
38-
tbl, err := i.checkOutTable(ctx, op.TableID)
38+
tbl, err := i.checkOutTable(ctx, op.Param.TableID)
3939
if err != nil {
4040
return err
4141
}
4242
setter := tablestorageparam.NewSetter(tbl, false /* isNewObject */)
43-
return setter.SetToStringValue(ctx, op.Name, op.Value)
43+
return setter.SetToStringValue(ctx, op.Param.Name, op.Param.Value)
4444
}
4545

4646
func (i *immediateVisitor) ResetTableStorageParam(
4747
ctx context.Context, op scop.ResetTableStorageParam,
4848
) error {
49-
tbl, err := i.checkOutTable(ctx, op.TableID)
49+
tbl, err := i.checkOutTable(ctx, op.Param.TableID)
5050
if err != nil {
5151
return err
5252
}
5353
setter := tablestorageparam.NewSetter(tbl, false /* isNewObject */)
54-
return setter.ResetToZeroValue(ctx, op.Name)
54+
return setter.ResetToZeroValue(ctx, op.Param.Name)
5555
}
5656

5757
func (d *deferredVisitor) UpdateTTLScheduleMetadata(

‎pkg/sql/schemachanger/scop/immediate_mutation.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1263,11 +1263,11 @@ type SetTableSchemaLocked struct {
12631263
// SetTableStorageParam sets a storage parameter on a table.
12641264
type SetTableStorageParam struct {
12651265
immediateMutationOp
1266-
scpb.TableStorageParam
1266+
Param scpb.TableStorageParam
12671267
}
12681268

12691269
// ResetTableStorageParam resets a storage parameter on a table to its default.
12701270
type ResetTableStorageParam struct {
12711271
immediateMutationOp
1272-
scpb.TableStorageParam
1272+
Param scpb.TableStorageParam
12731273
}

‎pkg/sql/schemachanger/scpb/elements.proto‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -970,7 +970,7 @@ message TableSchemaLocked {
970970
uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"];
971971
}
972972

973-
// TableStorageParam models storage parameter `schema_locked` of a table.
973+
// TableStorageParam models the storage parameters of a table other than `schema_locked` and RowLevelTTL-related ones.
974974
message TableStorageParam {
975975
uint32 table_id = 1 [(gogoproto.customname) = "TableID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/catid.DescID"];
976976
string name = 2;

‎pkg/sql/schemachanger/scplan/internal/opgen/opgen_table_storage_params.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ func init() {
1818
to(scpb.Status_PUBLIC,
1919
emit(func(this *scpb.TableStorageParam) *scop.SetTableStorageParam {
2020
return &scop.SetTableStorageParam{
21-
TableStorageParam: *protoutil.Clone(this).(*scpb.TableStorageParam),
21+
Param: *protoutil.Clone(this).(*scpb.TableStorageParam),
2222
}
2323
}),
2424
),
@@ -28,7 +28,7 @@ func init() {
2828
to(scpb.Status_ABSENT,
2929
emit(func(this *scpb.TableStorageParam) *scop.ResetTableStorageParam {
3030
return &scop.ResetTableStorageParam{
31-
TableStorageParam: *protoutil.Clone(this).(*scpb.TableStorageParam),
31+
Param: *protoutil.Clone(this).(*scpb.TableStorageParam),
3232
}
3333
}),
3434
),

‎pkg/sql/schemachanger/scplan/testdata/alter_table_set_storage_param‎

Lines changed: 72 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,10 @@ StatementPhase stage 1 of 1 with 1 MutationType op
1010
[[TableStorageParam:{DescID: 104, Name: exclude_data_from_backup, Value: true}, PUBLIC], ABSENT] -> PUBLIC
1111
ops:
1212
*scop.SetTableStorageParam
13-
{}
13+
Param:
14+
Name: exclude_data_from_backup
15+
TableID: 104
16+
Value: "true"
1417
PreCommitPhase stage 1 of 2 with 1 MutationType op
1518
transitions:
1619
[[TableStorageParam:{DescID: 104, Name: exclude_data_from_backup, Value: true}, PUBLIC], PUBLIC] -> ABSENT
@@ -22,7 +25,10 @@ PreCommitPhase stage 2 of 2 with 1 MutationType op
2225
[[TableStorageParam:{DescID: 104, Name: exclude_data_from_backup, Value: true}, PUBLIC], ABSENT] -> PUBLIC
2326
ops:
2427
*scop.SetTableStorageParam
25-
{}
28+
Param:
29+
Name: exclude_data_from_backup
30+
TableID: 104
31+
Value: "true"
2632

2733
ops
2834
ALTER TABLE defaultdb.foo SET (sql_stats_histogram_buckets_count = 64)
@@ -32,7 +38,10 @@ StatementPhase stage 1 of 1 with 1 MutationType op
3238
[[TableStorageParam:{DescID: 104, Name: sql_stats_histogram_buckets_count, Value: 64}, PUBLIC], ABSENT] -> PUBLIC
3339
ops:
3440
*scop.SetTableStorageParam
35-
{}
41+
Param:
42+
Name: sql_stats_histogram_buckets_count
43+
TableID: 104
44+
Value: "64"
3645
PreCommitPhase stage 1 of 2 with 1 MutationType op
3746
transitions:
3847
[[TableStorageParam:{DescID: 104, Name: sql_stats_histogram_buckets_count, Value: 64}, PUBLIC], PUBLIC] -> ABSENT
@@ -44,7 +53,10 @@ PreCommitPhase stage 2 of 2 with 1 MutationType op
4453
[[TableStorageParam:{DescID: 104, Name: sql_stats_histogram_buckets_count, Value: 64}, PUBLIC], ABSENT] -> PUBLIC
4554
ops:
4655
*scop.SetTableStorageParam
47-
{}
56+
Param:
57+
Name: sql_stats_histogram_buckets_count
58+
TableID: 104
59+
Value: "64"
4860

4961
ops
5062
ALTER TABLE defaultdb.foo SET (exclude_data_from_backup = true, sql_stats_histogram_buckets_count = 64)
@@ -55,9 +67,15 @@ StatementPhase stage 1 of 1 with 2 MutationType ops
5567
[[TableStorageParam:{DescID: 104, Name: sql_stats_histogram_buckets_count, Value: 64}, PUBLIC], ABSENT] -> PUBLIC
5668
ops:
5769
*scop.SetTableStorageParam
58-
{}
70+
Param:
71+
Name: exclude_data_from_backup
72+
TableID: 104
73+
Value: "true"
5974
*scop.SetTableStorageParam
60-
{}
75+
Param:
76+
Name: sql_stats_histogram_buckets_count
77+
TableID: 104
78+
Value: "64"
6179
PreCommitPhase stage 1 of 2 with 1 MutationType op
6280
transitions:
6381
[[TableStorageParam:{DescID: 104, Name: exclude_data_from_backup, Value: true}, PUBLIC], PUBLIC] -> ABSENT
@@ -71,9 +89,15 @@ PreCommitPhase stage 2 of 2 with 2 MutationType ops
7189
[[TableStorageParam:{DescID: 104, Name: sql_stats_histogram_buckets_count, Value: 64}, PUBLIC], ABSENT] -> PUBLIC
7290
ops:
7391
*scop.SetTableStorageParam
74-
{}
92+
Param:
93+
Name: exclude_data_from_backup
94+
TableID: 104
95+
Value: "true"
7596
*scop.SetTableStorageParam
76-
{}
97+
Param:
98+
Name: sql_stats_histogram_buckets_count
99+
TableID: 104
100+
Value: "64"
77101

78102
setup
79103
CREATE TABLE defaultdb.bar (x INT PRIMARY KEY) WITH (exclude_data_from_backup = true);
@@ -88,9 +112,15 @@ StatementPhase stage 1 of 1 with 2 MutationType ops
88112
[[TableStorageParam:{DescID: 105, Name: exclude_data_from_backup, Value: false}, PUBLIC], ABSENT] -> PUBLIC
89113
ops:
90114
*scop.ResetTableStorageParam
91-
{}
115+
Param:
116+
Name: exclude_data_from_backup
117+
TableID: 105
118+
Value: "true"
92119
*scop.SetTableStorageParam
93-
{}
120+
Param:
121+
Name: exclude_data_from_backup
122+
TableID: 105
123+
Value: "false"
94124
PreCommitPhase stage 1 of 2 with 1 MutationType op
95125
transitions:
96126
[[TableStorageParam:{DescID: 105, Name: exclude_data_from_backup, Value: true}, ABSENT], ABSENT] -> PUBLIC
@@ -104,9 +134,15 @@ PreCommitPhase stage 2 of 2 with 2 MutationType ops
104134
[[TableStorageParam:{DescID: 105, Name: exclude_data_from_backup, Value: false}, PUBLIC], ABSENT] -> PUBLIC
105135
ops:
106136
*scop.ResetTableStorageParam
107-
{}
137+
Param:
138+
Name: exclude_data_from_backup
139+
TableID: 105
140+
Value: "true"
108141
*scop.SetTableStorageParam
109-
{}
142+
Param:
143+
Name: exclude_data_from_backup
144+
TableID: 105
145+
Value: "false"
110146

111147
ops
112148
ALTER TABLE defaultdb.bar RESET (exclude_data_from_backup)
@@ -116,7 +152,10 @@ StatementPhase stage 1 of 1 with 1 MutationType op
116152
[[TableStorageParam:{DescID: 105, Name: exclude_data_from_backup, Value: true}, ABSENT], PUBLIC] -> ABSENT
117153
ops:
118154
*scop.ResetTableStorageParam
119-
{}
155+
Param:
156+
Name: exclude_data_from_backup
157+
TableID: 105
158+
Value: "true"
120159
PreCommitPhase stage 1 of 2 with 1 MutationType op
121160
transitions:
122161
[[TableStorageParam:{DescID: 105, Name: exclude_data_from_backup, Value: true}, ABSENT], ABSENT] -> PUBLIC
@@ -128,7 +167,10 @@ PreCommitPhase stage 2 of 2 with 1 MutationType op
128167
[[TableStorageParam:{DescID: 105, Name: exclude_data_from_backup, Value: true}, ABSENT], PUBLIC] -> ABSENT
129168
ops:
130169
*scop.ResetTableStorageParam
131-
{}
170+
Param:
171+
Name: exclude_data_from_backup
172+
TableID: 105
173+
Value: "true"
132174

133175
setup
134176
CREATE TABLE defaultdb.baz (y INT PRIMARY KEY) WITH (exclude_data_from_backup = true, sql_stats_histogram_buckets_count = 100);
@@ -143,9 +185,15 @@ StatementPhase stage 1 of 1 with 2 MutationType ops
143185
[[TableStorageParam:{DescID: 106, Name: sql_stats_histogram_buckets_count, Value: 100}, ABSENT], PUBLIC] -> ABSENT
144186
ops:
145187
*scop.ResetTableStorageParam
146-
{}
188+
Param:
189+
Name: exclude_data_from_backup
190+
TableID: 106
191+
Value: "true"
147192
*scop.ResetTableStorageParam
148-
{}
193+
Param:
194+
Name: sql_stats_histogram_buckets_count
195+
TableID: 106
196+
Value: "100"
149197
PreCommitPhase stage 1 of 2 with 1 MutationType op
150198
transitions:
151199
[[TableStorageParam:{DescID: 106, Name: exclude_data_from_backup, Value: true}, ABSENT], ABSENT] -> PUBLIC
@@ -159,6 +207,12 @@ PreCommitPhase stage 2 of 2 with 2 MutationType ops
159207
[[TableStorageParam:{DescID: 106, Name: sql_stats_histogram_buckets_count, Value: 100}, ABSENT], PUBLIC] -> ABSENT
160208
ops:
161209
*scop.ResetTableStorageParam
162-
{}
210+
Param:
211+
Name: exclude_data_from_backup
212+
TableID: 106
213+
Value: "true"
163214
*scop.ResetTableStorageParam
164-
{}
215+
Param:
216+
Name: sql_stats_histogram_buckets_count
217+
TableID: 106
218+
Value: "100"

0 commit comments

Comments
 (0)