Skip to content

Commit

Permalink
Merge cockroachdb#91704 cockroachdb#91867
Browse files Browse the repository at this point in the history 
91704: *: Improve constraints retrieval in table descriptor r=Xiang-Gu a=Xiang-Gu

This PR tries to improve how we retrieve constraints in a table descriptor. Previously,
it was mostly legacy code carries over from a while ago and nothing hasn't really
changed.
The main change is to introduce `catalog.Constraint` interface, similar to `catalog.Index`
and `catalog.Column`, as the preferred interface for constraint in this layer. Previously,
we would directly expose the underlying protobuf descriptor.

Commit 1 (easy): Rename `catalog.ConstraintToUpdate` to `catalog.Constraint`. 
It's good that we already have an interface that is suitable to be used for our effort.

Commit 2 (easy): Added methods in just renamed `catalog.Constraint` interface for
index-backed-constraints (i.e. PRIMARY KEY or UNIQUE);

Commit 3 (easy): Let `tabledesc.index` struct implement `catalog.Constraint` interface
as we will use it for index-backed-constraints.

Commit 4 (easy): Add a method in `catalog.Constraint` that gets validity of the constraint.

Commit 5 (moderate): Add logic (`ConstraintCache`) to pre-compute all constraints in a
table, categorized by type and validity, so that we can readily retrieve them later. This is the same
idea/technique used for managing index and columns (in `IndexCache` and `ColumnCache`).

Commit 6 (easy): Introduce the new, preferred methods in `catalog.TableDescriptor` to
retrieve constraints from a table.

Commit 7 (easy): Refactor signature of the existing `FindConstraintWithID` method to use
the newly added interface and retrieval methods.

Informs: cockroachdb#90840 
(this PR can unblock cockroachdb#90840)

Release note: None

91867: ui: change height of column selector r=maryliag a=maryliag

Previosuly, it was hard to identify there was more items on the columns selector, since the scrollbar is confugured by the user and might not show up right away (it will show once you hover with mouse and scroll).
This commit changes the height of the filter, making part of the next options to show up, hinting there is more options when scrolling.

Part Of cockroachdb#91763

Before
<img width="322" alt="Screen Shot 2022-11-14 at 2 59 39 PM" src="https://user-images.githubusercontent.com/1017486/201755400-1276e45b-62b8-44c0-a7ff-c337090ad94a.png">

After
<img width="308" alt="Screen Shot 2022-11-14 at 3 02 47 PM" src="https://user-images.githubusercontent.com/1017486/201755427-906e1c3b-e9fa-443b-9508-b2957b38d90b.png">


Release note (ui change): Change the height of column selector, so it can hint there are more options to be selected once scrolled.

Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: maryliag <[email protected]>
  • Loading branch information
@Xiang-Gu @maryliag
3 people committed Nov 15, 2022
3 parents 1e59412 + 25ace40 + fe2f0af commit 47c7b3a
Show file tree
Hide file tree
Showing 21 changed files with 665 additions and 74 deletions.
28 changes: 14 additions & 14 deletions pkg/sql/backfill.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,9 @@ func (sc *SchemaChanger) runBackfill(ctx context.Context) error {
var addedIndexes []descpb.IndexID
var temporaryIndexes []descpb.IndexID

var constraintsToDrop []catalog.ConstraintToUpdate
var constraintsToAddBeforeValidation []catalog.ConstraintToUpdate
var constraintsToValidate []catalog.ConstraintToUpdate
var constraintsToDrop []catalog.Constraint
var constraintsToAddBeforeValidation []catalog.Constraint
var constraintsToValidate []catalog.Constraint

var viewToRefresh catalog.MaterializedViewRefresh

Expand Down Expand Up @@ -405,7 +405,7 @@ func (sc *SchemaChanger) runBackfill(ctx context.Context) error {
// validation and be added directly. A Check Constraint can skip validation if it's
// created for a shard column internally.
func shouldSkipConstraintValidation(
tableDesc catalog.TableDescriptor, c catalog.ConstraintToUpdate,
tableDesc catalog.TableDescriptor, c catalog.Constraint,
) (bool, error) {
if !c.IsCheck() {
return false, nil
Expand Down Expand Up @@ -434,11 +434,11 @@ func shouldSkipConstraintValidation(
// the given constraint removed from it, and waits until the entire cluster is
// on the new version of the table descriptor. It returns the new table descs.
func (sc *SchemaChanger) dropConstraints(
ctx context.Context, constraints []catalog.ConstraintToUpdate,
ctx context.Context, constraints []catalog.Constraint,
) (map[descpb.ID]catalog.TableDescriptor, error) {
log.Infof(ctx, "dropping %d constraints", len(constraints))

fksByBackrefTable := make(map[descpb.ID][]catalog.ConstraintToUpdate)
fksByBackrefTable := make(map[descpb.ID][]catalog.Constraint)
for _, c := range constraints {
if c.IsForeignKey() {
id := c.ForeignKey().ReferencedTableID
Expand Down Expand Up @@ -577,11 +577,11 @@ func (sc *SchemaChanger) dropConstraints(
// given constraint added to it, and waits until the entire cluster is on
// the new version of the table descriptor.
func (sc *SchemaChanger) addConstraints(
ctx context.Context, constraints []catalog.ConstraintToUpdate,
ctx context.Context, constraints []catalog.Constraint,
) error {
log.Infof(ctx, "adding %d constraints", len(constraints))

fksByBackrefTable := make(map[descpb.ID][]catalog.ConstraintToUpdate)
fksByBackrefTable := make(map[descpb.ID][]catalog.Constraint)
for _, c := range constraints {
if c.IsForeignKey() {
id := c.ForeignKey().ReferencedTableID
Expand Down Expand Up @@ -717,7 +717,7 @@ func (sc *SchemaChanger) addConstraints(
// This operates over multiple goroutines concurrently and is thus not
// able to reuse the original kv.Txn safely, so it makes its own.
func (sc *SchemaChanger) validateConstraints(
ctx context.Context, constraints []catalog.ConstraintToUpdate,
ctx context.Context, constraints []catalog.Constraint,
) error {
if lease.TestingTableLeasesAreDisabled() {
return nil
Expand Down Expand Up @@ -1526,13 +1526,13 @@ func (e InvalidIndexesError) Error() string {
func ValidateCheckConstraint(
ctx context.Context,
tableDesc catalog.TableDescriptor,
constraint *descpb.ConstraintDetail,
constraint catalog.Constraint,
sessionData *sessiondata.SessionData,
runHistoricalTxn descs.HistoricalInternalExecTxnRunner,
execOverride sessiondata.InternalExecutorOverride,
) (err error) {
if constraint.CheckConstraint == nil {
return errors.AssertionFailedf("%v is not a check constraint", constraint.GetConstraintName())
if !constraint.IsCheck() {
return errors.AssertionFailedf("%v is not a check constraint", constraint.GetName())
}

tableDesc, err = tableDesc.MakeFirstMutationPublic(catalog.IgnoreConstraints)
Expand All @@ -1551,7 +1551,7 @@ func ValidateCheckConstraint(
defer func() { descriptors.ReleaseAll(ctx) }()

return ie.WithSyntheticDescriptors([]catalog.Descriptor{tableDesc}, func() error {
return validateCheckExpr(ctx, &semaCtx, txn, sessionData, constraint.CheckConstraint.Expr,
return validateCheckExpr(ctx, &semaCtx, txn, sessionData, constraint.Check().Expr,
tableDesc.(*tabledesc.Mutable), ie)
})
})
Expand Down Expand Up @@ -2352,7 +2352,7 @@ func runSchemaChangesInTxn(
// in the world of transactional schema changes.

// Collect constraint mutations to process later.
var constraintAdditionMutations []catalog.ConstraintToUpdate
var constraintAdditionMutations []catalog.Constraint

// We use a range loop here as the processing of some mutations
// such as the primary key swap mutations result in queueing more
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/backfill_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ import (
"github.com/stretchr/testify/require"
)

// constraintToUpdateForTest implements the catalog.ConstraintToUpdate interface.
// constraintToUpdateForTest implements the catalog.Constraint interface.
// It's only used for testing
type constraintToUpdateForTest struct {
catalog.ConstraintToUpdate
catalog.Constraint
desc *descpb.ConstraintToUpdate
}

Expand Down
16 changes: 15 additions & 1 deletion pkg/sql/catalog/descriptor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,13 +632,27 @@ type TableDescriptor interface {
// It's only non-nil if IsView is true.
GetDependsOnTypes() []descpb.ID

// AllConstraints returns all constraints from this table.
// A constraint is considered
// - active if its validity is VALIDATED;
// - inactive if its validity is VALIDATING or UNVALIDATED;
// - dropping if its validity is DROPPING;
AllConstraints() []Constraint
// AllActiveAndInactiveConstraints returns all active and inactive constraints from table.
AllActiveAndInactiveConstraints() []Constraint
// AllActiveConstraints returns all active constraints from table.
AllActiveConstraints() []Constraint

// GetConstraintInfoWithLookup returns a summary of all constraints on the
// table using the provided function to fetch a TableDescriptor from an ID.
// TODO (xiang): The following two legacy methods (`GetConstraintInfoWithLookup`
// and `GetConstraintInfo`) should be replaced with the methods above that
// retrieve constraints from the table and expose a `Constraint` interface.
GetConstraintInfoWithLookup(fn TableLookupFn) (map[string]descpb.ConstraintDetail, error)
// GetConstraintInfo returns a summary of all constraints on the table.
GetConstraintInfo() (map[string]descpb.ConstraintDetail, error)
// FindConstraintWithID returns a constraint given a constraint id.
FindConstraintWithID(id descpb.ConstraintID) (*descpb.ConstraintDetail, error)
FindConstraintWithID(id descpb.ConstraintID) (Constraint, error)

// GetUniqueWithoutIndexConstraints returns all the unique constraints defined
// on this table that are not enforced by an index.
Expand Down
190 changes: 190 additions & 0 deletions pkg/sql/catalog/descriptor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package catalog_test

import (
"sort"
"testing"

"github.com/cockroachdb/cockroach/pkg/sql/catalog"
Expand Down Expand Up @@ -66,3 +67,192 @@ func TestFormatSafeDescriptorProperties(t *testing.T) {
})
}
}

// TestConstraintRetrieval tests the following three method inside catalog.TableDescriptor interface.
// - AllConstraints() []Constraint
// - AllActiveAndInactiveConstraints() []Constraint
// - AllActiveConstraints() []Constraint
func TestConstraintRetrieval(t *testing.T) {
// Construct a table with the following constraints:
// - Primary Index: [ID_1:validated]
// - Indexes: [a non-unique index, ID_4:validated]
// - Checks: [ID_2:validated], [ID_3:unvalidated]
// - OutboundFKs: [ID_6:validated]
// - InboundFKs: [ID_5:validated]
// - UniqueWithoutIndexConstraints: [ID_7:dropping]
// - mutation slice: [ID_7:dropping:UniqueWithoutIndex, ID_8:validating:Check, ID_9:validating:UniqueIndex, a non-unique index]
primaryIndex := descpb.IndexDescriptor{
Unique: true,
ConstraintID: 1,
EncodingType: descpb.PrimaryIndexEncoding,
}

indexes := make([]descpb.IndexDescriptor, 2)
indexes[0] = descpb.IndexDescriptor{
Unique: false,
}
indexes[1] = descpb.IndexDescriptor{
Unique: true,
ConstraintID: 4,
}

checks := make([]*descpb.TableDescriptor_CheckConstraint, 2)
checks[0] = &descpb.TableDescriptor_CheckConstraint{
Validity: descpb.ConstraintValidity_Validated,
ConstraintID: 2,
}
checks[1] = &descpb.TableDescriptor_CheckConstraint{
Validity: descpb.ConstraintValidity_Unvalidated,
ConstraintID: 3,
}

outboundFKs := make([]descpb.ForeignKeyConstraint, 1)
outboundFKs[0] = descpb.ForeignKeyConstraint{
Validity: descpb.ConstraintValidity_Validated,
ConstraintID: 6,
}

inboundFKs := make([]descpb.ForeignKeyConstraint, 1)
inboundFKs[0] = descpb.ForeignKeyConstraint{
Validity: descpb.ConstraintValidity_Validated,
ConstraintID: 5,
}

uniqueWithoutIndexConstraints := make([]descpb.UniqueWithoutIndexConstraint, 1)
uniqueWithoutIndexConstraints[0] = descpb.UniqueWithoutIndexConstraint{
Validity: descpb.ConstraintValidity_Dropping,
ConstraintID: 7,
}

mutations := make([]descpb.DescriptorMutation, 4)
mutations[0] = descpb.DescriptorMutation{
Descriptor_: &descpb.DescriptorMutation_Constraint{
Constraint: &descpb.ConstraintToUpdate{
ConstraintType: descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX,
Name: "unique_on_k_without_index",
UniqueWithoutIndexConstraint: uniqueWithoutIndexConstraints[0],
},
},
State: descpb.DescriptorMutation_DELETE_ONLY,
Direction: descpb.DescriptorMutation_DROP,
}
mutations[1] = descpb.DescriptorMutation{
Descriptor_: &descpb.DescriptorMutation_Constraint{
Constraint: &descpb.ConstraintToUpdate{
ConstraintType: descpb.ConstraintToUpdate_CHECK,
Check: descpb.TableDescriptor_CheckConstraint{
Validity: descpb.ConstraintValidity_Validating,
ConstraintID: 8,
},
}},
State: descpb.DescriptorMutation_DELETE_ONLY,
Direction: descpb.DescriptorMutation_ADD,
}
mutations[2] = descpb.DescriptorMutation{
Descriptor_: &descpb.DescriptorMutation_Index{
Index: &descpb.IndexDescriptor{
Unique: true,
ConstraintID: 9,
}},
State: descpb.DescriptorMutation_DELETE_ONLY,
Direction: descpb.DescriptorMutation_ADD,
}
mutations[3] = descpb.DescriptorMutation{
Descriptor_: &descpb.DescriptorMutation_Index{
Index: &descpb.IndexDescriptor{
Unique: false,
}},
State: descpb.DescriptorMutation_DELETE_ONLY,
Direction: descpb.DescriptorMutation_ADD,
}

tableDesc := tabledesc.NewBuilder(&descpb.TableDescriptor{
PrimaryIndex: primaryIndex,
Indexes: indexes,
Checks: checks,
OutboundFKs: outboundFKs,
InboundFKs: inboundFKs,
UniqueWithoutIndexConstraints: uniqueWithoutIndexConstraints,
Mutations: mutations,
}).BuildImmutable().(catalog.TableDescriptor)

t.Run("test-AllConstraints", func(t *testing.T) {
all := tableDesc.AllConstraints()
sort.Slice(all, func(i, j int) bool {
return all[i].GetConstraintID() < all[j].GetConstraintID()
})
require.Equal(t, len(all), 9)
checkIndexBackedConstraint(t, all[0], 1, descpb.ConstraintValidity_Validated, descpb.PrimaryIndexEncoding)
checkNonIndexBackedConstraint(t, all[1], 2, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_CHECK)
checkNonIndexBackedConstraint(t, all[2], 3, descpb.ConstraintValidity_Unvalidated, descpb.ConstraintToUpdate_CHECK)
checkIndexBackedConstraint(t, all[3], 4, descpb.ConstraintValidity_Validated, descpb.SecondaryIndexEncoding)
checkNonIndexBackedConstraint(t, all[4], 5, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_FOREIGN_KEY)
checkNonIndexBackedConstraint(t, all[5], 6, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_FOREIGN_KEY)
checkNonIndexBackedConstraint(t, all[6], 7, descpb.ConstraintValidity_Dropping, descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX)
checkNonIndexBackedConstraint(t, all[7], 8, descpb.ConstraintValidity_Validating, descpb.ConstraintToUpdate_CHECK)
checkIndexBackedConstraint(t, all[8], 9, descpb.ConstraintValidity_Validating, descpb.SecondaryIndexEncoding)
})

t.Run("test-AllActiveAndInactiveConstraints", func(t *testing.T) {
allActiveAndInactive := tableDesc.AllActiveAndInactiveConstraints()
sort.Slice(allActiveAndInactive, func(i, j int) bool {
return allActiveAndInactive[i].GetConstraintID() < allActiveAndInactive[j].GetConstraintID()
})
require.Equal(t, len(allActiveAndInactive), 8)
checkIndexBackedConstraint(t, allActiveAndInactive[0], 1, descpb.ConstraintValidity_Validated, descpb.PrimaryIndexEncoding)
checkNonIndexBackedConstraint(t, allActiveAndInactive[1], 2, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_CHECK)
checkNonIndexBackedConstraint(t, allActiveAndInactive[2], 3, descpb.ConstraintValidity_Unvalidated, descpb.ConstraintToUpdate_CHECK)
checkIndexBackedConstraint(t, allActiveAndInactive[3], 4, descpb.ConstraintValidity_Validated, descpb.SecondaryIndexEncoding)
checkNonIndexBackedConstraint(t, allActiveAndInactive[4], 5, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_FOREIGN_KEY)
checkNonIndexBackedConstraint(t, allActiveAndInactive[5], 6, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_FOREIGN_KEY)
checkNonIndexBackedConstraint(t, allActiveAndInactive[6], 8, descpb.ConstraintValidity_Validating, descpb.ConstraintToUpdate_CHECK)
checkIndexBackedConstraint(t, allActiveAndInactive[7], 9, descpb.ConstraintValidity_Validating, descpb.SecondaryIndexEncoding)
})

t.Run("test-AllActiveConstraints", func(t *testing.T) {
allActive := tableDesc.AllActiveConstraints()
sort.Slice(allActive, func(i, j int) bool {
return allActive[i].GetConstraintID() < allActive[j].GetConstraintID()
})
require.Equal(t, len(allActive), 5)
checkIndexBackedConstraint(t, allActive[0], 1, descpb.ConstraintValidity_Validated, descpb.PrimaryIndexEncoding)
checkNonIndexBackedConstraint(t, allActive[1], 2, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_CHECK)
checkIndexBackedConstraint(t, allActive[2], 4, descpb.ConstraintValidity_Validated, descpb.SecondaryIndexEncoding)
checkNonIndexBackedConstraint(t, allActive[3], 5, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_FOREIGN_KEY)
checkNonIndexBackedConstraint(t, allActive[4], 6, descpb.ConstraintValidity_Validated, descpb.ConstraintToUpdate_FOREIGN_KEY)
})
}

// checkIndexBackedConstraint ensures `c` (PRIMARY KEY or UNIQUE)
// has the expected ID, validity, and type.
//
// `expectedEncodingType = secondaryIndexEncoding` is used for
// UNIQUE constraint type.
func checkIndexBackedConstraint(
t *testing.T,
c catalog.Constraint,
expectedID descpb.ConstraintID,
expectedValidity descpb.ConstraintValidity,
expectedEncodingType descpb.IndexDescriptorEncodingType,
) {
require.Equal(t, expectedID, c.GetConstraintID())
require.Equal(t, expectedValidity, c.GetConstraintValidity())
require.Equal(t, expectedEncodingType, c.IndexDesc().EncodingType)
if expectedEncodingType == descpb.SecondaryIndexEncoding {
require.Equal(t, true, c.IndexDesc().Unique)
}
}

// checkNonIndexBackedConstraint ensures `c` (CHECK, FK, or UNIQUE_WITHOUT_INDEX)
// has the expected ID, validity, and type.
func checkNonIndexBackedConstraint(
t *testing.T,
c catalog.Constraint,
expectedID descpb.ConstraintID,
expectedValidity descpb.ConstraintValidity,
expectedType descpb.ConstraintToUpdate_ConstraintType,
) {
require.Equal(t, expectedID, c.GetConstraintID())
require.Equal(t, expectedValidity, c.GetConstraintValidity())
require.Equal(t, expectedType, c.ConstraintToUpdateDesc().ConstraintType)
}
Loading

0 comments on commit 47c7b3a

Please sign in to comment.