*: Improve constraints retrieval in table descriptor by Xiang-Gu · Pull Request #91704 · cockroachdb/cockroach

Xiang-Gu · 2022-11-10T21:29:12Z

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: #90840
(this PR can unblock #90840)

Release note: None

cockroach-teamcity · 2022-11-10T21:29:28Z

This change is

ajwerner

Reviewed 6 of 6 files at r1, 4 of 4 files at r2, 2 of 2 files at r3, 4 of 4 files at r4, 4 of 4 files at r5, 9 of 9 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Xiang-Gu)

pkg/sql/catalog/tabledesc/mutation.go line 200 at r3 (raw file):

		return c.UniqueWithoutIndex().Validity
	} else {
		panic("unknown constraint type")

please panic an assertion failed error, the string will get redacted

postamar

Ok, so, this is sort of OK, but it requires a fair bit of follow-up work, which I'll be happy to do.

postamar · 2022-11-14T16:58:52Z

pkg/sql/catalog/table_elements.go

 	// there is one.
 	UniqueWithoutIndex() descpb.UniqueWithoutIndexConstraint

+	// PrimaryKey returns the underlying index descriptor backing the UNIQUE constraint,


nit: fix comment

postamar · 2022-11-14T17:01:38Z

pkg/sql/catalog/table_elements.go

 	UniqueWithoutIndex() descpb.UniqueWithoutIndexConstraint

+	// PrimaryKey returns the underlying index descriptor backing the UNIQUE constraint,
+	// if there is one.


nit: can you add a TODO(postamar): return interface instead of protobuf comment here and for Unique() please?

I will just do it here myself.

postamar · 2022-11-14T17:03:48Z

pkg/sql/catalog/tabledesc/constraint.go

+	allActiveAndInactiveFKs []catalog.Constraint
+	allActiveFKs            []catalog.Constraint
+
+	allUniqueWithoutIndexs                  []catalog.Constraint


nit: s/allUniqueWithoutIndexs/allUniqueWithoutIndexes; same for 2 lines below.

postamar · 2022-11-14T17:23:29Z

pkg/sql/catalog/descpb/structured.proto

+  // - VALIDATING when the index is being added (i.e. live on mutation slice with ADD direction);
+  // - DROPPING when the index is being dropped (i.e. live on mutation slice with DROP direction);
+  // - UNVALIDATED in no case!
+  optional ConstraintValidity index_backed_constraint_validity = 9 [(gogoproto.nullable) = false];


I'm very concerned about this change to the protobuf (the two new fields + two new enum values). It's dangerous because it implies a change in what's serialized to storage. It's also not really necessary, why not modify the index struct in tabledesc to implement catalog.Constraint instead? Everything should be there already.

Is the perceived obstacle to doing this that catalog.Constraint has a ConstraintToUpdateDesc() *descpb.ConstraintToUpdate method? I took a look and it seems to me that it could be removed. It's used for pretty-printing and for accessing the constraint subtypes like c.ConstraintToUpdateDesc().Check and for c.ConstraintToUpdateDesc().ConstraintType.

I will change as your suggested! However, I don't think we can get rid of ConstraintToUpdateDesc() because this is needed if we want to modify the underlying the protobuf. Now with index implementing Constraint, we will need to add another method, maybe called IndexDesc() *descpb.IndexDescriptor, if the to-be-modified constraint is backed by an index.

One such scanrio of needing to modify the underlying protobuf is setting the name of a constraint. This will show up in both RENAME CONSTRAINT or, more generally, the adding/dropping path of a ConstraintName element.

postamar · 2022-11-14T17:31:07Z

pkg/sql/catalog/tabledesc/constraint.go

+		dest = append(dest, c)
+		constraintIDsInDest[c.GetConstraintID()] = true
+		return dest
+	}


Is it at all possible for a constraint to present itself more than once?

yes, I knew of one scenario: when dropping a check constraint, we would find that constraint in the Checks slice, mark its validity to DROPPING without removing it from the Checks slice. We would also enqueue a mutation for this check constraint with DROP direction. Here, this dropping check constraint will be found both in Checks slice and on Mutations slice.

Thanks! I feared it would be something like this.

postamar · 2022-11-14T17:36:32Z

pkg/sql/catalog/tabledesc/constraint.go

+	}
+
+	return &c
+}


This might not have been obvious to you but this function is in the hot path and its current implementation is going to cause noticeable performance regressions.

I'll address this after this merges, it's important but it's not interesting and shouldn't block you from making progress on the declarative schema changer.

sgtm, I will leave it as is for this PR.

postamar

LGTM, nicely done.

Xiang-Gu · 2022-11-14T22:42:18Z

TFTR!

bors r+

Xiang-Gu · 2022-11-14T22:56:12Z

bors r-

Some pieces are missing and let me fill them up.

craig · 2022-11-14T22:56:14Z

Canceled.

This PR renames the following interface and struct: 1. `catalog.ConstraintToUpdate` interface is now `catalog.Constraint` 2. `tabledesc.constraintToUpdate` struct is now `tabledesc.constraint`

1. We let `tabledesc.index` struct implement `catalog.Constraint` interface so that it is used for any index-backed-constraints (PRIMARY KEY or UNIQUE); 2. We implement a `constraintCache` inside a table wrapper that was pre-populated and is ready for re-use. It allows us to easily retrieve various sets of constraints (by kind and/or by validity) from the table. 3. We expose three methods in `catalog.TableDescriptor` interface to retrieve various constraints in this table using this cache.

This commit refactor `FindConstraintWithID` method inside the `catalog.TableDescriptor` in the following two ways: 1. Change the return type to the newly introduced `catalog.Constraint` instead of a protobuf struct; 2. Change the implementation slightly to also search dropping constraints. Previously, it will skip those constraints. The remaining changes in this commit are consequential changes due to this function signature change. All of them are peripheral and does not affect functionality.

Xiang-Gu · 2022-11-14T23:12:46Z

Now I think it's ready to be merged again!

bors r+

craig · 2022-11-14T23:53:26Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-11-15T01:09:20Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-11-15T03:18:59Z

Build succeeded:

Bazel Essential CI (Cockroach)

Xiang-Gu force-pushed the add-constraint-interface branch 2 times, most recently from 637fa4c to e91fe19 Compare November 11, 2022 19:36

Xiang-Gu marked this pull request as ready for review November 11, 2022 19:37

Xiang-Gu requested a review from a team November 11, 2022 19:37

This was referenced Nov 14, 2022

schemachanger: enable adding/dropping path of constraint name #90840

Closed

catalog, tabledesc: Add methods to retrieve all constraints in a table #91063

Closed

ajwerner approved these changes Nov 14, 2022

View reviewed changes

postamar reviewed Nov 14, 2022

View reviewed changes

Xiang-Gu force-pushed the add-constraint-interface branch from e91fe19 to 7e8c489 Compare November 14, 2022 20:26

postamar approved these changes Nov 14, 2022

View reviewed changes

Xiang-Gu force-pushed the add-constraint-interface branch from 7e8c489 to b5f89e5 Compare November 14, 2022 22:27

Xiang-Gu added 3 commits November 14, 2022 18:05

catalog, tabledesc: Rename an interface and its backing struct

1dbc43e

This PR renames the following interface and struct: 1. `catalog.ConstraintToUpdate` interface is now `catalog.Constraint` 2. `tabledesc.constraintToUpdate` struct is now `tabledesc.constraint`

Xiang-Gu force-pushed the add-constraint-interface branch from b5f89e5 to 25ace40 Compare November 14, 2022 23:05

craig bot merged commit 47c7b3a into cockroachdb:master Nov 15, 2022

Xiang-Gu deleted the add-constraint-interface branch November 15, 2022 15:28

postamar mentioned this pull request Nov 15, 2022

catalog: add Constraint interface subtypes, improve constraint caching in tabledesc.immutable #91918

Closed

Conversation

Xiang-Gu commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 10, 2022

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

Xiang-Gu commented Nov 14, 2022

Uh oh!

Xiang-Gu commented Nov 14, 2022

Uh oh!

craig bot commented Nov 14, 2022

Uh oh!

Xiang-Gu commented Nov 14, 2022

Uh oh!

craig bot commented Nov 14, 2022

Uh oh!

craig bot commented Nov 15, 2022

Uh oh!

craig bot commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xiang-Gu commented Nov 10, 2022 •

edited

Loading

Xiang-Gu Nov 14, 2022 •

edited

Loading