Skip to content

Commit

Permalink
sql/schemachanger: cleaned some dep rules in declarative schema changer
Browse files Browse the repository at this point in the history 
Three dependency rules were found to be problematic:
  1. "dependents removed after index no longer public": this one
     turns out to have its logic "backwards" and we fixed it;
  2. "column type removed right before column when not dropping
     relation": this dep rule was never used; it was removed.
  3. "partial predicate removed right before secondary index when not
     dropping relation": similarly, this rule was never used; it
     was removed.

Consequently to the removal of 2 and 3 above, we can also remove the
some peripheral codes used by 2 and 3.

Release note: None
  • Loading branch information
@Xiang-Gu
Xiang-Gu committed Jun 14, 2022
1 parent 7d9562d commit 056c5d3
Show file tree
Hide file tree
Showing 24 changed files with 1,524 additions and 417 deletions.
13 changes: 0 additions & 13 deletions pkg/ccl/schemachangerccl/testdata/decomp/multiregion
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ ElementState:
width: 64
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 110
Status: PUBLIC
Expand All @@ -209,7 +208,6 @@ ElementState:
width: 64
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 110
Status: PUBLIC
Expand All @@ -236,7 +234,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 110
Status: PUBLIC
Expand All @@ -263,7 +260,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 110
Status: PUBLIC
Expand Down Expand Up @@ -428,7 +424,6 @@ ElementState:
width: 64
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 109
Status: PUBLIC
Expand All @@ -455,7 +450,6 @@ ElementState:
width: 64
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 109
Status: PUBLIC
Expand All @@ -482,7 +476,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 109
Status: PUBLIC
Expand All @@ -509,7 +502,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 109
Status: PUBLIC
Expand Down Expand Up @@ -694,7 +686,6 @@ ElementState:
width: 64
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 108
Status: PUBLIC
Expand All @@ -721,7 +712,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 108
Status: PUBLIC
Expand Down Expand Up @@ -751,7 +741,6 @@ ElementState:
width: 0
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 108
Status: PUBLIC
Expand All @@ -778,7 +767,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 108
Status: PUBLIC
Expand All @@ -805,7 +793,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 108
Status: PUBLIC
Expand Down
9 changes: 0 additions & 9 deletions pkg/ccl/schemachangerccl/testdata/decomp/partitioning
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,6 @@ ElementState:
width: 64
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 104
Status: PUBLIC
Expand All @@ -213,7 +212,6 @@ ElementState:
width: 64
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 104
Status: PUBLIC
Expand All @@ -240,7 +238,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 104
Status: PUBLIC
Expand All @@ -267,7 +264,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 104
Status: PUBLIC
Expand All @@ -294,7 +290,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 104
Status: PUBLIC
Expand Down Expand Up @@ -517,7 +512,6 @@ ElementState:
width: 64
familyId: 0
isNullable: false
isRelationBeingDropped: false
isVirtual: false
tableId: 105
Status: PUBLIC
Expand All @@ -544,7 +538,6 @@ ElementState:
width: 64
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 105
Status: PUBLIC
Expand All @@ -571,7 +564,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 105
Status: PUBLIC
Expand All @@ -598,7 +590,6 @@ ElementState:
width: 0
familyId: 0
isNullable: true
isRelationBeingDropped: false
isVirtual: false
tableId: 105
Status: PUBLIC
Expand Down
14 changes: 0 additions & 14 deletions pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@ upsert descriptor #108
+ family: IntFamily
+ oid: 20
+ width: 64
+ isRelationBeingDropped: true
+ tableId: 108
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -309,7 +308,6 @@ upsert descriptor #108
+ oid: 100105
+ udtMetadata:
+ arrayTypeOid: 100107
+ isRelationBeingDropped: true
+ tableId: 108
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -356,7 +354,6 @@ upsert descriptor #108
+ family: DecimalFamily
+ oid: 1700
+ isNullable: true
+ isRelationBeingDropped: true
+ tableId: 108
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -390,7 +387,6 @@ upsert descriptor #108
+ family: OidFamily
+ oid: 26
+ isNullable: true
+ isRelationBeingDropped: true
+ tableId: 108
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -665,7 +661,6 @@ upsert descriptor #108
- family: IntFamily
- oid: 20
- width: 64
- isRelationBeingDropped: true
- tableId: 108
- metadata:
- sourceElementId: 1
Expand Down Expand Up @@ -702,7 +697,6 @@ upsert descriptor #108
- oid: 100105
- udtMetadata:
- arrayTypeOid: 100107
- isRelationBeingDropped: true
- tableId: 108
- metadata:
- sourceElementId: 1
Expand Down Expand Up @@ -749,7 +743,6 @@ upsert descriptor #108
- family: DecimalFamily
- oid: 1700
- isNullable: true
- isRelationBeingDropped: true
- tableId: 108
- metadata:
- sourceElementId: 1
Expand Down Expand Up @@ -783,7 +776,6 @@ upsert descriptor #108
- family: OidFamily
- oid: 26
- isNullable: true
- isRelationBeingDropped: true
- tableId: 108
- metadata:
- sourceElementId: 1
Expand Down Expand Up @@ -1092,7 +1084,6 @@ upsert descriptor #109
+ family: IntFamily
+ oid: 20
+ width: 64
+ isRelationBeingDropped: true
+ tableId: 109
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -1126,7 +1117,6 @@ upsert descriptor #109
+ family: DecimalFamily
+ oid: 1700
+ isNullable: true
+ isRelationBeingDropped: true
+ tableId: 109
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -1160,7 +1150,6 @@ upsert descriptor #109
+ family: OidFamily
+ oid: 26
+ isNullable: true
+ isRelationBeingDropped: true
+ tableId: 109
+ metadata:
+ sourceElementId: 1
Expand Down Expand Up @@ -1383,7 +1372,6 @@ upsert descriptor #109
- family: IntFamily
- oid: 20
- width: 64
- isRelationBeingDropped: true
- tableId: 109
- metadata:
- sourceElementId: 1
Expand Down Expand Up @@ -1417,7 +1405,6 @@ upsert descriptor #109
- family: DecimalFamily
- oid: 1700
- isNullable: true
- isRelationBeingDropped: true
- tableId: 109
- metadata:
- sourceElementId: 1
Expand Down Expand Up @@ -1451,7 +1438,6 @@ upsert descriptor #109
- family: OidFamily
- oid: 26
- isNullable: true
- isRelationBeingDropped: true
- tableId: 109
- metadata:
- sourceElementId: 1
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/opt/exec/execbuilder/testdata/show_trace
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_sequence.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func DropSequence(b BuildCtx, n *tree.DropSequence) {
scpb.ForEachSequenceOwner(
undroppedBackrefs(b, seq.SequenceID),
func(_ scpb.Status, _ scpb.TargetStatus, so *scpb.SequenceOwner) {
dropElement(b, so)
b.Drop(so)
},
)
toCheckBackrefs = append(toCheckBackrefs, seq.SequenceID)
Expand Down
17 changes: 3 additions & 14 deletions pkg/sql/schemachanger/scbuild/internal/scbuildstmt/helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,12 @@ func dropRestrictDescriptor(b BuildCtx, id catid.DescID) (hasChanged bool) {
return
}
b.CheckPrivilege(e, privilege.DROP)
dropElement(b, e)
b.Drop(e)
hasChanged = true
})
return hasChanged
}

func dropElement(b BuildCtx, e scpb.Element) {
// TODO(postamar): remove this dirty hack ASAP, see column/index dep rules.
switch t := e.(type) {
case *scpb.ColumnType:
t.IsRelationBeingDropped = true
case *scpb.SecondaryIndexPartial:
t.IsRelationBeingDropped = true
}
b.Drop(e)
}

// dropCascadeDescriptor contains the common logic for dropping something with
// CASCADE.
func dropCascadeDescriptor(b BuildCtx, id catid.DescID) {
Expand Down Expand Up @@ -112,7 +101,7 @@ func dropCascadeDescriptor(b BuildCtx, id catid.DescID) {
// Don't actually drop any elements of virtual schemas.
return
}
dropElement(b, e)
b.Drop(e)
switch t := e.(type) {
case *scpb.EnumType:
dropCascadeDescriptor(next, t.ArrayTypeID)
Expand Down Expand Up @@ -146,7 +135,7 @@ func dropCascadeDescriptor(b BuildCtx, id catid.DescID) {
*scpb.ForeignKeyConstraint,
*scpb.SequenceOwner,
*scpb.DatabaseRegionConfig:
dropElement(b, e)
b.Drop(e)
}
})
}
Expand Down
Loading

0 comments on commit 056c5d3

Please sign in to comment.