This is one of these instances where we're more careful to not re-use a name. Things can get quite confusing otherwise when rolling back a schema change involving a name collision.

This is one of these instances where the adding and dropping states are handled more uniformly, to no adverse effect from what I can tell.

So many calls to GetConstraintInfo were really about finding a constraint by name. Though, notably, GetConstraintInfo would not return the full set of constraints (no dropping unique indexes for instance). In this case here I don't think that mattered but I'm willing to bet this caused some subtle undetected bugs at some other GetConstraintInfo call sites.

I think (but I'm not sure) that omitting dropped unique-without-indexes was a bug.

See function definition for details but we no longer pass slices of descpb.UniqueConstraint interfaces, because we got rid of that interface.

This has been superseded by catalog.UniqueConstraint, in effect.

Moving this further ahead was necessary because we got rid of CheckUniqueConstraints, but the price to pay was lots of noise in validate_test.go.

index.NumKeyColumns() == 0 can legitimately happen for the primary index of a virtual table. Of course, such tables don't have primary indexes at all, so I'm not sure how much I like the fact that we pretend that we do, but this is not the time or place to deal with this.

I just don't think that this should succeed. Use another name.

nit: Is the comment stale? Why do we say "...if the mutation is on a check constraint..."?

good catch, thanks

I suggest we always have a line like var _ catalog.ConstraintProvider = constraintBase{} to enforce implementation of an interface in compile time. Same is true for all other structs that we claim to have implemented some interface.

Okay, I see you are doing this for the specific constraint type (like checkConstraint). Great!

Thanks but yeah it's a good point. IIRC I try to have the assertions on the leaves of the type hierarchy.

should we panic with assertionFailedf here instead of returning 0? Or it's the caller's responsibility to check whether return is 0 and handles it appropriately if so?

Your comment made me look what we actually do in catalog.Index's implementation and I'd always assumed we did bound checks but it turns out that we don't.

I'll do what catalog.Index does and remove these checks. I remember that back when that was implemented, this matter had been discussed. Evidently the consensus that came out of that was that we preferred triggering a runtime panic when going out of bounds. I'm OK with that.

Unfortunately, the legacy code did a bad job about the state machine of constraints while it's still on the mutation slice. I think it's better to check the constraint mutation's validity instead of the MutationDescriptor_State, so I suggest we instead just do return c.desc.validity != dropping. But this again iterates one of my comments above that maybe we can get rid of this method and just expose GetConstraintValidity.

To illustrate what I mean above a bit more: consider adding a check constraint. When we enqueue the check constraint onto the mutation slice, its MutationDescriptor_State will be (unfortunately) DELETE_ONLY but its validity is VALIDATING, which should be considered as Enforced but this method now will incorrectly returns false.

A constraint's validity and whether it's enforced are two different things:

• the validity concerns the data that was present before the constraint is added;
• a constraint being enforced or not concerns the data that appears after the constraint is added.

why &c, not c?

In go, an interface is basically a pair of pointers, so it makes sense for the underlying type of what the interface is wrapping to also be a pointer type.

I'm not a huge fan of go's pointer magic in general, and hardly an expert.

nit: the naming seems to suggest all desc.Checks are enforced checks and all mutations.checks are not enforced checks, which isn't the case, right?

A check on the mutation with VALIDATING validity is considered enforced, right?

See above.

why do we check uwi.NumKeyColumns() additionally? In what scenario will we have a non-nil uwi but 0 key columns?

Virtual tables. It's dumb, in those cases we should have GetPrimaryIndex() return nil, yet we don't. Yet more tech debt.

I still feel like a comment block like this that summarizes all the constraints we put on this contrived table is helpful.

OK I'll add it back.

Why do you think those methods belong here? It seems to me more appropriately to define them just as function that takes in a (ck|fk|uwoi) and return some property of that input constraint. Do we need anything from the table to serve those questions?

Good question. The table is required to map the column ID to a catalog.Column, and I really prefer using the latter instead of IDs whenever possible.

See the similar []Column-returning methods taking indexes. These can be quite powerful and removing this layer of indirection of the column ID makes for some more expressive code at the call sites. See informationSchemaConstraintColumnUsageTable` for an example. Arguably some of them are never called, but if they're there it's because they used to, and perhaps will be again.