sql: introduce MutableTableDescriptor struct

[eriktrinh]

Replace the type alias which made MutableTableDescriptor and
TableDescriptor equivalent types with a struct that embeds
TableDescriptor. This forces any write of a table descriptor for schema
changes to use this struct.

Release note: None

[cockroach-teamcity]

This change is 

[vivekmenezes]

I'm very impressed with the work you've put together here by introducing this new type. It's going to help the system get more robust. I think that the second commit is not relevant to this PR and should be removed from the PR. You can send it to me as a separate PR.

Thanks!

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/create_view.go, line 161 at r1 (raw file):

		if backRefMutable == nil {
			backRefMutable = &sqlbase.MutableTableDescriptor{TableDescriptor: updated.desc}
		}

Why do you need to lookup the uncommitted tables and then create a new MutableTableDescriptor here?

---

pkg/sql/descriptor.go, line 132 at r1 (raw file):

	} else {
		mutDesc, ok = descriptor.(*sqlbase.MutableTableDescriptor)
	}

The above code seems odd

---

pkg/sql/physical_schema_accessors.go, line 146 at r1 (raw file):

	}
	return nil, nil, nil
}

Why is the change made here required for this PR?

---

pkg/sql/resolver.go, line 112 at r1 (raw file):

	}
	return desc.(*MutableTableDescriptor), nil
}

great! Love it!

---

pkg/sql/resolver.go, line 155 at r1 (raw file):

		return &MutableTableDescriptor{
			TableDescriptor: descI.(*TableDescriptor),
		}, nil

You probably want to add a TODO here to remove this in a subsequent PR.
I actually feel more comfortable using a function NewMutableTableDescriptor() so we can easily track where new mutable table descriptors are created.

---

pkg/sql/schema_accessors.go, line 51 at r1 (raw file):

	// interface definitions below more intuitive.
	ObjectDescriptor = sqlbase.TableDescriptor
	// MutableTableDescriptor is provided for convenience and to make the

excellent!

---

pkg/sql/schema_accessors.go, line 84 at r1 (raw file):

	// IsSequence returns true if the descriptor actually describes a
	// Sequence resource rather than a Table.
	IsSequence() bool

It does appear that these ought to be methods on TableDescriptor. So users would do obj.TableDesc().IsTable()

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/create_view.go, line 161 at r1 (raw file):

Previously, vivekmenezes wrote…

Why do you need to lookup the uncommitted tables and then create a new MutableTableDescriptor here?

We look up the uncommitted tables because the referenced table descriptors are updated and written, which should happen on the MutableTableDescriptor as well.

---

pkg/sql/descriptor.go, line 132 at r1 (raw file):

Previously, vivekmenezes wrote…

The above code seems odd

Done. Hopefully this is more readable.

---

pkg/sql/physical_schema_accessors.go, line 146 at r1 (raw file):

Previously, vivekmenezes wrote…

Why is the change made here required for this PR?

We don't want to assign nil to desc in the old code and return it because the returned ObjectDescriptor will be a non-nil interface value with an underlying nil pointer.

---

pkg/sql/resolver.go, line 112 at r1 (raw file):

Previously, vivekmenezes wrote…

great! Love it!

Ack.

---

pkg/sql/resolver.go, line 155 at r1 (raw file):

Previously, vivekmenezes wrote…

You probably want to add a TODO here to remove this in a subsequent PR.
I actually feel more comfortable using a function NewMutableTableDescriptor() so we can easily track where new mutable table descriptors are created.

I think it would be worth it to add a second commit to this PR which does so.

---

pkg/sql/schema_accessors.go, line 51 at r1 (raw file):

Previously, vivekmenezes wrote…

excellent!

Ack.

---

pkg/sql/schema_accessors.go, line 84 at r1 (raw file):

Previously, vivekmenezes wrote…

It does appear that these ought to be methods on TableDescriptor. So users would do obj.TableDesc().IsTable()

Done.

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/descriptor.go, line 129 at r2 (raw file):

	var mutDesc *sqlbase.MutableTableDescriptor
	switch d := descriptor.(type) {
	case *sqlbase.MutableTableDescriptor:

how can a descriptor.(type) ever be a MutableTableDescriptor?

---

pkg/sql/resolver.go, line 155 at r1 (raw file):

Previously, eriktrinh (Erik Trinh) wrote…

I think it would be worth it to add a second commit to this PR which does so.

this looks fine for now

[vivekmenezes]

This looks good. Feel free to incorporate the feedback and merge. Thanks for making it happen!

Reviewable status: complete! 0 of 0 LGTMs obtained

[vivekmenezes]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[eriktrinh]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/descriptor.go, line 129 at r2 (raw file):

Previously, vivekmenezes wrote…

how can a descriptor.(type) ever be a MutableTableDescriptor?

this is called in create_table and create_view where it uses created MutableTableDescriptors which implements DescriptorProto

[eriktrinh]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)